From 4484eb9049557e82289306494ec8e1c231c2c243 Mon Sep 17 00:00:00 2001 From: Junior Martinez <67972863+jmartinez-silabs@users.noreply.github.com> Date: Tue, 8 Oct 2024 11:24:44 -0400 Subject: [PATCH] Address comments --- .../GenericPlatformManagerImpl_CMSISOS.h | 14 ++-- .../GenericPlatformManagerImpl_CMSISOS.ipp | 71 ++++++------------- 2 files changed, 30 insertions(+), 55 deletions(-) diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.h b/src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.h index 0e9e6a9cd4df7d..e40281527abbb5 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.h +++ b/src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.h @@ -47,14 +47,14 @@ class GenericPlatformManagerImpl_CMSISOS : public GenericPlatformManagerImpl mShouldRunEventLoop; #if defined(CHIP_CONFIG_CMSISOS_USE_STATIC_QUEUE) && CHIP_CONFIG_CMSISOS_USE_STATIC_QUEUE diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.ipp b/src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.ipp index a844b2a7fc2299..b75fcbe0c24a6f 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.ipp +++ b/src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.ipp @@ -42,34 +42,26 @@ namespace Internal { template CHIP_ERROR GenericPlatformManagerImpl_CMSISOS::_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 { @@ -85,11 +77,8 @@ CHIP_ERROR GenericPlatformManagerImpl_CMSISOS::_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 { @@ -100,11 +89,7 @@ CHIP_ERROR GenericPlatformManagerImpl_CMSISOS::_InitChipStack(void) #endif // Call up to the base class _InitChipStack() to perform the bulk of the initialization. - err = GenericPlatformManagerImpl::_InitChipStack(); - SuccessOrExit(err); - -exit: - return err; + return GenericPlatformManagerImpl::_InitChipStack(); } template @@ -141,7 +126,7 @@ bool GenericPlatformManagerImpl_CMSISOS::_IsChipStackLockedByCurrentT template CHIP_ERROR GenericPlatformManagerImpl_CMSISOS::_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) @@ -155,9 +140,6 @@ CHIP_ERROR GenericPlatformManagerImpl_CMSISOS::_PostEvent(const ChipD template void GenericPlatformManagerImpl_CMSISOS::_RunEventLoop(void) { - CHIP_ERROR err; - ChipDeviceEvent event; - // Lock the CHIP stack. StackLock lock; @@ -177,7 +159,7 @@ void GenericPlatformManagerImpl_CMSISOS::_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) @@ -188,9 +170,9 @@ void GenericPlatformManagerImpl_CMSISOS::_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(DeviceLayer::SystemLayer()).HandlePlatformTimer(); + CHIP_ERROR err = static_cast(DeviceLayer::SystemLayer()).HandlePlatformTimer(); if (err != CHIP_NO_ERROR) { ChipLogError(DeviceLayer, "Error handling CHIP timers: %" CHIP_ERROR_FORMAT, err.Format()); @@ -199,7 +181,7 @@ void GenericPlatformManagerImpl_CMSISOS::_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; } @@ -207,6 +189,7 @@ void GenericPlatformManagerImpl_CMSISOS::_RunEventLoop(void) // 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. @@ -236,14 +219,14 @@ void GenericPlatformManagerImpl_CMSISOS::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 uint32_t GenericPlatformManagerImpl_CMSISOS::SyncNextChipTimerHandling() { - uint32_t elapsedTime; + uint32_t elapsedTime = 0; uint32_t timerBaseTime = mNextTimerBaseTime; uint32_t currentTime = osKernelGetTickCount(); if (currentTime < timerBaseTime) @@ -273,21 +256,15 @@ template CHIP_ERROR GenericPlatformManagerImpl_CMSISOS::_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 @@ -300,11 +277,9 @@ void GenericPlatformManagerImpl_CMSISOS::_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()) {