Skip to content

Commit

Permalink
Remove one kind of Timer (#8488)
Browse files Browse the repository at this point in the history 
* Remove one kind of Timer

#### Problem

System::Layer has two functionally equivalent kinds of timer,
which is a maintenance burden. One of them is very little
used, and not currently supported on all platforms.

#### Change overview

Remove the kind of timer that takes a `Callback<>`.

#### Testing

Revised the unit test. The changed calls are both in ESP32,
which is tested in CI using QEMU.

* review
  • Loading branch information
@kpschoedel
kpschoedel authored Jul 22, 2021
1 parent 250d5f7 commit 773af5a
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 148 deletions.
8 changes: 3 additions & 5 deletions src/platform/ESP32/BLEManagerImpl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class BLEManagerImpl final : public BLEManager,
private Ble::BleApplicationDelegate
{
public:
BLEManagerImpl();
BLEManagerImpl() {}

private:
// Allow the BLEManager interface class to delegate method calls to
Expand Down Expand Up @@ -199,12 +199,10 @@ class BLEManagerImpl final : public BLEManager,
static constexpr uint32_t kAdvertiseTimeout = CHIP_DEVICE_CONFIG_BLE_ADVERTISING_TIMEOUT;
static constexpr uint32_t kFastAdvertiseTimeout = CHIP_DEVICE_CONFIG_BLE_ADVERTISING_INTERVAL_CHANGE_TIME;
uint64_t mAdvertiseStartTime;
chip::Callback::Callback<> mAdvertiseTimerCallback;
chip::Callback::Callback<> mFastAdvertiseTimerCallback;

static void HandleFastAdvertisementTimer(void * context);
static void HandleFastAdvertisementTimer(System::Layer * systemLayer, void * context, CHIP_ERROR aError);
void HandleFastAdvertisementTimer();
static void HandleAdvertisementTimer(void * context);
static void HandleAdvertisementTimer(System::Layer * systemLayer, void * context, CHIP_ERROR aError);
void HandleAdvertisementTimer();

#if CONFIG_BT_BLUEDROID_ENABLED
Expand Down
12 changes: 4 additions & 8 deletions src/platform/ESP32/bluedroid/BLEManagerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,6 @@ const uint16_t CHIPoBLEGATTAttrCount = sizeof(CHIPoBLEGATTAttrs) / sizeof(CHIPoB

BLEManagerImpl BLEManagerImpl::sInstance;

BLEManagerImpl::BLEManagerImpl() :
mAdvertiseTimerCallback(HandleAdvertisementTimer, this), mFastAdvertiseTimerCallback(HandleFastAdvertisementTimer, this)
{}

CHIP_ERROR BLEManagerImpl::_Init()
{
CHIP_ERROR err;
Expand Down Expand Up @@ -181,8 +177,8 @@ CHIP_ERROR BLEManagerImpl::_SetAdvertisingEnabled(bool val)
if (val)
{
mAdvertiseStartTime = System::Clock::GetMonotonicMilliseconds();
SystemLayer.StartTimer(kAdvertiseTimeout, &mAdvertiseTimerCallback);
SystemLayer.StartTimer(kFastAdvertiseTimeout, &mFastAdvertiseTimerCallback);
ReturnErrorOnFailure(SystemLayer.StartTimer(kAdvertiseTimeout, HandleAdvertisementTimer, this));
ReturnErrorOnFailure(SystemLayer.StartTimer(kFastAdvertiseTimeout, HandleFastAdvertisementTimer, this));
}
mFlags.Set(Flags::kFastAdvertisingEnabled, val);
mFlags.Set(Flags::kAdvertisingRefreshNeeded, 1);
Expand All @@ -192,7 +188,7 @@ CHIP_ERROR BLEManagerImpl::_SetAdvertisingEnabled(bool val)
return err;
}

void BLEManagerImpl::HandleAdvertisementTimer(void * context)
void BLEManagerImpl::HandleAdvertisementTimer(System::Layer * systemLayer, void * context, CHIP_ERROR aError)
{
static_cast<BLEManagerImpl *>(context)->HandleAdvertisementTimer();
}
Expand All @@ -208,7 +204,7 @@ void BLEManagerImpl::HandleAdvertisementTimer()
}
}

void BLEManagerImpl::HandleFastAdvertisementTimer(void * context)
void BLEManagerImpl::HandleFastAdvertisementTimer(System::Layer * systemLayer, void * context, CHIP_ERROR aError)
{
static_cast<BLEManagerImpl *>(context)->HandleFastAdvertisementTimer();
}
Expand Down
12 changes: 4 additions & 8 deletions src/platform/ESP32/nimble/BLEManagerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,6 @@ const ble_uuid128_t UUID_CHIPoBLEChar_TX = {

BLEManagerImpl BLEManagerImpl::sInstance;

BLEManagerImpl::BLEManagerImpl() :
mAdvertiseTimerCallback(HandleAdvertisementTimer, this), mFastAdvertiseTimerCallback(HandleFastAdvertisementTimer, this)
{}

const struct ble_gatt_svc_def BLEManagerImpl::CHIPoBLEGATTAttrs[] = {
{ .type = BLE_GATT_SVC_TYPE_PRIMARY,
.uuid = (ble_uuid_t *) (&ShortUUID_CHIPoBLEService),
Expand Down Expand Up @@ -174,8 +170,8 @@ CHIP_ERROR BLEManagerImpl::_SetAdvertisingEnabled(bool val)
if (val)
{
mAdvertiseStartTime = System::Clock::GetMonotonicMilliseconds();
SystemLayer.StartTimer(kAdvertiseTimeout, &mAdvertiseTimerCallback);
SystemLayer.StartTimer(kFastAdvertiseTimeout, &mFastAdvertiseTimerCallback);
ReturnErrorOnFailure(SystemLayer.StartTimer(kAdvertiseTimeout, HandleAdvertisementTimer, this));
ReturnErrorOnFailure(SystemLayer.StartTimer(kFastAdvertiseTimeout, HandleFastAdvertisementTimer, this));
}

mFlags.Set(Flags::kFastAdvertisingEnabled, val);
Expand All @@ -187,7 +183,7 @@ CHIP_ERROR BLEManagerImpl::_SetAdvertisingEnabled(bool val)
return err;
}

void BLEManagerImpl::HandleAdvertisementTimer(void * context)
void BLEManagerImpl::HandleAdvertisementTimer(System::Layer * systemLayer, void * context, CHIP_ERROR aError)
{
static_cast<BLEManagerImpl *>(context)->HandleAdvertisementTimer();
}
Expand All @@ -203,7 +199,7 @@ void BLEManagerImpl::HandleAdvertisementTimer()
}
}

void BLEManagerImpl::HandleFastAdvertisementTimer(void * context)
void BLEManagerImpl::HandleFastAdvertisementTimer(System::Layer * systemLayer, void * context, CHIP_ERROR aError)
{
static_cast<BLEManagerImpl *>(context)->HandleFastAdvertisementTimer();
}
Expand Down
118 changes: 0 additions & 118 deletions src/system/SystemLayer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,41 +61,6 @@
namespace chip {
namespace System {

namespace {

Clock::MonotonicMilliseconds GetTimestamp(const Callback::Cancelable * timer)
{
Clock::MonotonicMilliseconds timestamp;
static_assert(sizeof(timestamp) <= sizeof(timer->mInfo), "mInfo is too small for timestamp");
memcpy(&timestamp, &timer->mInfo, sizeof(timestamp));
return timestamp;
}

void SetTimestamp(Callback::Cancelable * timer, Clock::MonotonicMilliseconds timestamp)
{
static_assert(sizeof(timestamp) <= sizeof(timer->mInfo), "mInfo is too small for timestamp");
memcpy(&timer->mInfo, &timestamp, sizeof(timestamp));
}

bool TimerReady(const Clock::MonotonicMilliseconds timestamp, const Callback::Cancelable * timer)
{
return !Timer::IsEarlier(timestamp, GetTimestamp(timer));
}

int TimerCompare(void * p, const Callback::Cancelable * a, const Callback::Cancelable * b)
{
(void) p;

Clock::MonotonicMilliseconds timeA = GetTimestamp(a);
Clock::MonotonicMilliseconds timeB = GetTimestamp(b);

return (timeA > timeB) ? 1 : (timeA < timeB) ? -1 : 0;
}

} // namespace

using namespace ::chip::Callback;

Layer::Layer() : mLayerState(kLayerState_NotInitialized), mPlatformData(nullptr)
{
#if CHIP_SYSTEM_CONFIG_USE_LWIP
Expand Down Expand Up @@ -181,54 +146,6 @@ CHIP_ERROR Layer::NewTimer(Timer *& aTimerPtr)
return CHIP_NO_ERROR;
}

/**
* @brief
* This method starts a one-shot timer.
*
* @note
* Only a single timer is allowed to be started with the same @a aComplete and @a aAppState
* arguments. If called with @a aComplete and @a aAppState identical to an existing timer,
* the currently-running timer will first be cancelled.
*
* @param[in] aMilliseconds Expiration time in milliseconds.
* @param[in] aCallback A pointer to the Callback that fires when the timer expires
*
* @return CHIP_NO_ERROR On success.
* @return Other Value indicating timer failed to start.
*
*/
void Layer::StartTimer(uint32_t aMilliseconds, chip::Callback::Callback<> * aCallback)
{
#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
if (mDispatchQueue != nullptr)
{
ChipLogError(chipSystemLayer, "%s is not supported with libdispatch", __PRETTY_FUNCTION__);
chipDie();
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

assertChipStackLockedByCurrentThread();

Cancelable * ca = aCallback->Cancel();

SetTimestamp(ca, Clock::GetMonotonicMilliseconds() + aMilliseconds);

mTimerCallbacks.InsertBy(ca, TimerCompare, nullptr);

#if CHIP_SYSTEM_CONFIG_USE_LWIP
if (mTimerCallbacks.First() == ca)
{
// this is the new earliest timer and so the timer needs (re-)starting provided that
// the system is not currently processing expired timers, in which case it is left to
// HandleExpiredTimers() to re-start the timer.
if (!mTimerComplete)
{
StartPlatformTimer(aMilliseconds);
}
}
#endif // CHIP_SYSTEM_CONFIG_USE_LWIP
}

/**
* @brief
* This method starts a one-shot timer.
Expand Down Expand Up @@ -348,26 +265,6 @@ CHIP_ERROR Layer::ScheduleWork(TimerCompleteFunct aComplete, void * aAppState)
return lReturn;
}

/**
* @brief
* Run any timers that are due based on input current time
*/
void Layer::DispatchTimerCallbacks(const Clock::MonotonicMilliseconds aCurrentTime)
{
// dispatch TimerCallbacks
Cancelable ready;

mTimerCallbacks.DequeueBy(TimerReady, static_cast<uint64_t>(aCurrentTime), ready);

while (ready.mNext != &ready)
{
// one-shot
chip::Callback::Callback<> * cb = chip::Callback::Callback<>::FromCancelable(ready.mNext);
cb->Cancel();
cb->mCall(cb->mContext);
}
}

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK

bool Layer::GetTimeout(struct timeval & aSleepTime)
Expand Down Expand Up @@ -399,17 +296,6 @@ bool Layer::GetTimeout(struct timeval & aSleepTime)
}
}

// check for an earlier callback timer, too
if (lAwakenTime != kCurrentTime)
{
Cancelable * ca = mTimerCallbacks.First();
if (ca != nullptr && !Timer::IsEarlier(kCurrentTime, GetTimestamp(ca)))
{
anyTimer = true;
lAwakenTime = GetTimestamp(ca);
}
}

const Clock::MonotonicMilliseconds kSleepTime = lAwakenTime - kCurrentTime;
aSleepTime.tv_sec = static_cast<time_t>(kSleepTime / 1000);
aSleepTime.tv_usec = static_cast<suseconds_t>((kSleepTime % 1000) * 1000);
Expand Down Expand Up @@ -437,8 +323,6 @@ void Layer::HandleTimeout()
}
}

DispatchTimerCallbacks(kCurrentTime);

#if CHIP_SYSTEM_CONFIG_POSIX_LOCKING
this->mHandleSelectThread = PTHREAD_NULL;
#endif // CHIP_SYSTEM_CONFIG_POSIX_LOCKING
Expand Down Expand Up @@ -679,8 +563,6 @@ CHIP_ERROR Layer::HandlePlatformTimer()

lReturn = Timer::HandleExpiredTimers(*this);

DispatchTimerCallbacks(Clock::GetMonotonicMilliseconds());

SuccessOrExit(lReturn);

exit:
Expand Down
4 changes: 0 additions & 4 deletions src/system/SystemLayer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,6 @@ class DLL_EXPORT Layer

CHIP_ERROR NewTimer(Timer *& aTimerPtr);

void StartTimer(uint32_t aMilliseconds, chip::Callback::Callback<> * aCallback);
void DispatchTimerCallbacks(Clock::MonotonicMilliseconds aCurrentTime);

using TimerCompleteFunct = Timer::OnCompleteFunct;
// typedef void (*TimerCompleteFunct)(Layer * aLayer, void * aAppState, CHIP_ERROR aError);
CHIP_ERROR StartTimer(uint32_t aMilliseconds, TimerCompleteFunct aComplete, void * aAppState);
Expand Down Expand Up @@ -165,7 +162,6 @@ class DLL_EXPORT Layer
private:
LayerState mLayerState;
void * mPlatformData;
chip::Callback::CallbackDeque mTimerCallbacks;
Clock mClock;

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
Expand Down
5 changes: 0 additions & 5 deletions src/system/tests/TestSystemTimer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ class TestContext
return;
}

mLayer->StartTimer(0, &mGreedyTimer);
mNumTimersHandled++;
}
static void GreedyTimer(void * p)
Expand Down Expand Up @@ -127,7 +126,6 @@ void HandleTimer10Success(Layer * inetLayer, void * aState, CHIP_ERROR aError)
static void CheckOverflow(nlTestSuite * inSuite, void * aContext)
{
uint32_t timeout_overflow_0ms = 652835029;
uint32_t timeout_overflow_1ms = 1958505088;
uint32_t timeout_10ms = 10;

TestContext & lContext = *static_cast<TestContext *>(aContext);
Expand All @@ -136,8 +134,6 @@ static void CheckOverflow(nlTestSuite * inSuite, void * aContext)
sOverflowTestDone = false;

lSys.StartTimer(timeout_overflow_0ms, HandleTimerFailed, aContext);
chip::Callback::Callback<> cb(TimerFailed, aContext);
lSys.StartTimer(timeout_overflow_1ms, &cb);
lSys.StartTimer(timeout_10ms, HandleTimer10Success, aContext);

while (!sOverflowTestDone)
Expand Down Expand Up @@ -175,7 +171,6 @@ static void CheckStarvation(nlTestSuite * inSuite, void * aContext)
struct timeval sleepTime;

lSys.StartTimer(0, HandleGreedyTimer, aContext);
lSys.StartTimer(0, &lContext.mGreedyTimer);

sleepTime.tv_sec = 0;
sleepTime.tv_usec = 1000; // 1 ms tick
Expand Down

0 comments on commit 773af5a

Please sign in to comment.