Address comments

Author: jmartinez-silabs

src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.h

@@ -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.
@@ -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

src/include/platform/internal/GenericPlatformManagerImpl_CMSISOS.ipp

@@ -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
     {
@@ -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
     {
@@ -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>
@@ -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)
@@ -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;
 
@@ -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)
@@ -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());
@@ -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.
@@ -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)
@@ -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
@@ -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())
     {