Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@jmartinez-silabs
jmartinez-silabs committed Oct 8, 2024
1 parent 69797bc commit 4484eb9
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 55 deletions.
14 changes: 7 additions & 7 deletions src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ class GenericPlatformManagerImpl_CMSISOS : public GenericPlatformManagerImpl<Imp
{

protected:
osMutexId_t mChipStackLock = NULL;
osMessageQueueId_t mChipEventQueue = NULL;
osThreadId_t mEventLoopTask = NULL;
osMutexId_t mChipStackLock = nullptr;
osMessageQueueId_t mChipEventQueue = nullptr;
osThreadId_t mEventLoopTask = nullptr;
bool mChipTimerActive;

#if defined(CHIP_DEVICE_CONFIG_ENABLE_BG_EVENT_PROCESSING) && CHIP_DEVICE_CONFIG_ENABLE_BG_EVENT_PROCESSING
osMessageQueueId_t mBackgroundEventQueue = NULL;
osThreadId_t mBackgroundEventLoopTask = NULL;
osMessageQueueId_t mBackgroundEventQueue = nullptr;
osThreadId_t mBackgroundEventLoopTask = nullptr;
#endif

// ===== Methods that implement the PlatformManager abstract interface.
Expand Down Expand Up @@ -90,8 +90,8 @@ class GenericPlatformManagerImpl_CMSISOS : public GenericPlatformManagerImpl<Imp

static void EventLoopTaskMain(void * pvParameter);
uint32_t SyncNextChipTimerHandling();
uint32_t mNextTimerBaseTime;
uint32_t mNextTimerDurationTicks;
uint32_t mNextTimerBaseTime = 0;
uint32_t mNextTimerDurationTicks = 0;
std::atomic<bool> mShouldRunEventLoop;

#if defined(CHIP_CONFIG_CMSISOS_USE_STATIC_QUEUE) && CHIP_CONFIG_CMSISOS_USE_STATIC_QUEUE
Expand Down
71 changes: 23 additions & 48 deletions src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.ipp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,34 +42,26 @@ namespace Internal {
template <class ImplClass>
CHIP_ERROR GenericPlatformManagerImpl_CMSISOS<ImplClass>::_InitChipStack(void)
{
CHIP_ERROR err = CHIP_NO_ERROR;

mNextTimerBaseTime = osKernelGetTickCount();
mNextTimerDurationTicks = 0;
mChipTimerActive = false;

// We support calling Shutdown followed by InitChipStack, because some tests
// do that. To keep things simple for existing consumers, we keep not
// destroying our lock and queue in shutdown, but rather check whether they
// do that. To keep things simple for existing consumers, we do not
// destroy our lock and queue at shutdown, but rather check whether they
// already exist here before trying to create them.
if (mChipStackLock == nullptr)
{
mChipStackLock = osMutexNew(&mChipStackMutexAttr);
if (mChipStackLock == nullptr)
{
ChipLogError(DeviceLayer, "Failed to create CHIP stack lock");
ExitNow(err = CHIP_ERROR_NO_MEMORY);
}
VerifyOrReturnError(mChipStackLock != nullptr, CHIP_ERROR_NO_MEMORY,
ChipLogError(DeviceLayer, "Failed to create CHIP stack lock"));
}

if (mChipEventQueue == nullptr)
{
mChipEventQueue = osMessageQueueNew(CHIP_DEVICE_CONFIG_MAX_EVENT_QUEUE_SIZE, sizeof(ChipDeviceEvent), mEventQueueAttrPtr);
if (mChipEventQueue == nullptr)
{
ChipLogError(DeviceLayer, "Failed to allocate CHIP main event queue");
ExitNow(err = CHIP_ERROR_NO_MEMORY);
}
VerifyOrReturnError(mChipEventQueue != nullptr, CHIP_ERROR_NO_MEMORY,
ChipLogError(DeviceLayer, "Failed to allocate CHIP main event queue"));
}
else
{
Expand All @@ -85,11 +77,8 @@ CHIP_ERROR GenericPlatformManagerImpl_CMSISOS<ImplClass>::_InitChipStack(void)
{
mBackgroundEventQueue =
osMessageQueueNew(CHIP_DEVICE_CONFIG_BG_MAX_EVENT_QUEUE_SIZE, sizeof(ChipDeviceEvent), mBgQueueAttrPtr);
if (mBackgroundEventQueue == nullptr)
{
ChipLogError(DeviceLayer, "Failed to allocate CHIP background event queue");
ExitNow(err = CHIP_ERROR_NO_MEMORY);
}
VerifyOrReturnError(mBackgroundEventQueue != nullptr, CHIP_ERROR_NO_MEMORY,
ChipLogError(DeviceLayer, "Failed to allocate CHIP background event queue"));
}
else
{
Expand All @@ -100,11 +89,7 @@ CHIP_ERROR GenericPlatformManagerImpl_CMSISOS<ImplClass>::_InitChipStack(void)
#endif

// Call up to the base class _InitChipStack() to perform the bulk of the initialization.
err = GenericPlatformManagerImpl<ImplClass>::_InitChipStack();
SuccessOrExit(err);

exit:
return err;
return GenericPlatformManagerImpl<ImplClass>::_InitChipStack();
}

template <class ImplClass>
Expand Down Expand Up @@ -141,7 +126,7 @@ bool GenericPlatformManagerImpl_CMSISOS<ImplClass>::_IsChipStackLockedByCurrentT
template <class ImplClass>
CHIP_ERROR GenericPlatformManagerImpl_CMSISOS<ImplClass>::_PostEvent(const ChipDeviceEvent * event)
{
VerifyOrReturnError(mChipEventQueue != nullptr, CHIP_ERROR_INTERNAL);
VerifyOrReturnError(mChipEventQueue != nullptr, CHIP_ERROR_UNINITIALIZED);

osStatus_t status = osMessageQueuePut(mChipEventQueue, event, osPriorityNormal, 1);
if (status != osOK)
Expand All @@ -155,9 +140,6 @@ CHIP_ERROR GenericPlatformManagerImpl_CMSISOS<ImplClass>::_PostEvent(const ChipD
template <class ImplClass>
void GenericPlatformManagerImpl_CMSISOS<ImplClass>::_RunEventLoop(void)
{
CHIP_ERROR err;
ChipDeviceEvent event;

// Lock the CHIP stack.
StackLock lock;

Expand All @@ -177,7 +159,7 @@ void GenericPlatformManagerImpl_CMSISOS<ImplClass>::_RunEventLoop(void)
{
// Adjust the base time and remaining duration for the next scheduled timer based on the
// amount of time that has elapsed since it was started.
// When the timer's expiration time elapsed, Handle the platform Timer
// When the timer's expiration time elapses, Handle the platform Timer
// else wait for a queue event for timer remaining time.
waitTimeInTicks = SyncNextChipTimerHandling();
if (waitTimeInTicks == 0)
Expand All @@ -188,9 +170,9 @@ void GenericPlatformManagerImpl_CMSISOS<ImplClass>::_RunEventLoop(void)

// Call into the system layer to dispatch the callback functions for all timers
// that have expired.
// TODO We use the same SystemLayer implementation than FreeRTOS, Nothing in it is freeRTOS specific. Maybe rename
// TODO We use the same SystemLayer implementation as FreeRTOS, Nothing in it is freeRTOS specific. We should
// it.
err = static_cast<System::LayerImplFreeRTOS &>(DeviceLayer::SystemLayer()).HandlePlatformTimer();
CHIP_ERROR err = static_cast<System::LayerImplFreeRTOS &>(DeviceLayer::SystemLayer()).HandlePlatformTimer();
if (err != CHIP_NO_ERROR)
{
ChipLogError(DeviceLayer, "Error handling CHIP timers: %" CHIP_ERROR_FORMAT, err.Format());
Expand All @@ -199,14 +181,15 @@ void GenericPlatformManagerImpl_CMSISOS<ImplClass>::_RunEventLoop(void)
}
else
{
// No CHIP timers are active, so wait indefinitely for an event to arrive on the event
// No CHIP timers are active, so we wait indefinitely for an event to arrive on the event
// queue.
waitTimeInTicks = osWaitForever;
}

// Unlock the CHIP stack, allowing other threads to enter CHIP while
// the event loop thread is sleeping.
StackUnlock unlock;
ChipDeviceEvent event;
osStatus_t eventReceived = osMessageQueueGet(mChipEventQueue, &event, nullptr, waitTimeInTicks);

// If an event was received, dispatch it and continue until the queue is empty.
Expand Down Expand Up @@ -236,14 +219,14 @@ void GenericPlatformManagerImpl_CMSISOS<ImplClass>::EventLoopTaskMain(void * pvP

/**
* @brief Calculate the elapsed time of the active chip platform timer since it has been started,
* as set in mNextTimerBaseTime, and adjust its remaining time in mNextTimerDurationTicks member
* as set in mNextTimerBaseTime, and adjust its remaining time with the mNextTimerDurationTicks member
*
* @return The next Timer remaining time in ticks
*/
template <class ImplClass>
uint32_t GenericPlatformManagerImpl_CMSISOS<ImplClass>::SyncNextChipTimerHandling()
{
uint32_t elapsedTime;
uint32_t elapsedTime = 0;
uint32_t timerBaseTime = mNextTimerBaseTime;
uint32_t currentTime = osKernelGetTickCount();
if (currentTime < timerBaseTime)
Expand Down Expand Up @@ -273,21 +256,15 @@ template <class ImplClass>
CHIP_ERROR GenericPlatformManagerImpl_CMSISOS<ImplClass>::_PostBackgroundEvent(const ChipDeviceEvent * event)
{
#if defined(CHIP_DEVICE_CONFIG_ENABLE_BG_EVENT_PROCESSING) && CHIP_DEVICE_CONFIG_ENABLE_BG_EVENT_PROCESSING
if (mBackgroundEventQueue == NULL)
{
return CHIP_ERROR_INTERNAL;
}
VerifyOrReturnError(mBackgroundEventQueue != nullptr, CHIP_ERROR_UNINITIALIZED);
if (!(event->Type == DeviceEventType::kCallWorkFunct || event->Type == DeviceEventType::kNoOp))
{
return CHIP_ERROR_INVALID_ARGUMENT;
}

osStatus_t status = osMessageQueuePut(mBackgroundEventQueue, event, osPriorityNormal, 1);
if (status != osOK)
{
ChipLogError(DeviceLayer, "Failed to post event to CHIP background event queue");
return CHIP_ERROR_NO_MEMORY;
}
VerifyOrReturnError(status == osOk, CHIP_ERROR_NO_MEMORY,
ChipLogError(DeviceLayer, "Failed to post event to CHIP background event queue"));
return CHIP_NO_ERROR;
#else
// Use foreground event loop for background events
Expand All @@ -300,11 +277,9 @@ void GenericPlatformManagerImpl_CMSISOS<ImplClass>::_RunBackgroundEventLoop(void
{
#if defined(CHIP_DEVICE_CONFIG_ENABLE_BG_EVENT_PROCESSING) && CHIP_DEVICE_CONFIG_ENABLE_BG_EVENT_PROCESSING
bool oldShouldRunBackgroundEventLoop = false;
if (!mShouldRunBackgroundEventLoop.compare_exchange_strong(oldShouldRunBackgroundEventLoop /* expected */, true /* desired */))
{
ChipLogError(DeviceLayer, "Error trying to run the background event loop while it is already running");
return;
}
VerifyOrReturn(
mShouldRunBackgroundEventLoop.compare_exchange_strong(oldShouldRunBackgroundEventLoop /* expected */, true /* desired */),
ChipLogError(DeviceLayer, "Error trying to run the background event loop while it is already running"));

while (mShouldRunBackgroundEventLoop.load())
{
Expand Down

0 comments on commit 4484eb9

Please sign in to comment.