Address review comments.

project-chip · Sep 7, 2022 · e33d6fb · e33d6fb
1 parent 86dc30f
commit e33d6fb
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 28 deletions.
diff --git a/.restyled.yaml b/.restyled.yaml
@@ -98,6 +98,7 @@ restylers:
           - "**/*.c"
           - "**/*.cc"
           - "**/*.cpp"
+          - "**/*.ipp"
           - "**/*.cxx"
           - "**/*.c++"
           - "**/*.C"
@@ -137,6 +138,7 @@ restylers:
           - "**/*.c"
           - "**/*.cc"
           - "**/*.cpp"
+          - "**/*.ipp"
           - "**/*.cxx"
           - "**/*.c++"
           - "**/*.C"

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h
@@ -60,9 +60,9 @@ class GenericPlatformManagerImpl_FreeRTOS : public GenericPlatformManagerImpl<Im
 protected:
     TimeOut_t mNextTimerBaseTime;
     TickType_t mNextTimerDurationTicks;
-    SemaphoreHandle_t mChipStackLock;
-    QueueHandle_t mChipEventQueue;
-    TaskHandle_t mEventLoopTask;
+    SemaphoreHandle_t mChipStackLock = NULL;
+    QueueHandle_t mChipEventQueue    = NULL;
+    TaskHandle_t mEventLoopTask      = NULL;
     bool mChipTimerActive;
 
     // ===== Methods that implement the PlatformManager abstract interface.

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.ipp b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.ipp
@@ -46,31 +46,44 @@ CHIP_ERROR GenericPlatformManagerImpl_FreeRTOS<ImplClass>::_InitChipStack(void)
 
     vTaskSetTimeOutState(&mNextTimerBaseTime);
     mNextTimerDurationTicks = 0;
-    mEventLoopTask          = NULL;
-    mChipTimerActive        = false;
+    // TODO: This nulling out of mEventLoopTask should happen when we shut down
+    // the task, not here!
+    mEventLoopTask   = NULL;
+    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
+    // already exist here before trying to create them.
+
+    if (mChipStackLock == NULL)
+    {
 #if defined(CHIP_CONFIG_FREERTOS_USE_STATIC_SEMAPHORE) && CHIP_CONFIG_FREERTOS_USE_STATIC_SEMAPHORE
-    mChipStackLock = xSemaphoreCreateMutexStatic(&mChipStackLockMutex);
+        mChipStackLock = xSemaphoreCreateMutexStatic(&mChipStackLockMutex);
 #else
-    mChipStackLock  = xSemaphoreCreateMutex();
+        mChipStackLock  = xSemaphoreCreateMutex();
 #endif // CHIP_CONFIG_FREERTOS_USE_STATIC_SEMAPHORE
 
-    if (mChipStackLock == NULL)
-    {
-        ChipLogError(DeviceLayer, "Failed to create CHIP stack lock");
-        ExitNow(err = CHIP_ERROR_NO_MEMORY);
+        if (mChipStackLock == NULL)
+        {
+            ChipLogError(DeviceLayer, "Failed to create CHIP stack lock");
+            ExitNow(err = CHIP_ERROR_NO_MEMORY);
+        }
     }
 
+    if (mChipEventQueue == NULL)
+    {
 #if defined(CHIP_CONFIG_FREERTOS_USE_STATIC_QUEUE) && CHIP_CONFIG_FREERTOS_USE_STATIC_QUEUE
-    mChipEventQueue =
-        xQueueCreateStatic(CHIP_DEVICE_CONFIG_MAX_EVENT_QUEUE_SIZE, sizeof(ChipDeviceEvent), mEventQueueBuffer, &mEventQueueStruct);
+        mChipEventQueue = xQueueCreateStatic(CHIP_DEVICE_CONFIG_MAX_EVENT_QUEUE_SIZE, sizeof(ChipDeviceEvent), mEventQueueBuffer,
+                                             &mEventQueueStruct);
 #else
-    mChipEventQueue = xQueueCreate(CHIP_DEVICE_CONFIG_MAX_EVENT_QUEUE_SIZE, sizeof(ChipDeviceEvent));
+        mChipEventQueue = xQueueCreate(CHIP_DEVICE_CONFIG_MAX_EVENT_QUEUE_SIZE, sizeof(ChipDeviceEvent));
 #endif
-    if (mChipEventQueue == NULL)
-    {
-        ChipLogError(DeviceLayer, "Failed to allocate CHIP event queue");
-        ExitNow(err = CHIP_ERROR_NO_MEMORY);
+        if (mChipEventQueue == NULL)
+        {
+            ChipLogError(DeviceLayer, "Failed to allocate CHIP event queue");
+            ExitNow(err = CHIP_ERROR_NO_MEMORY);
+        }
     }
 
     mShouldRunEventLoop.store(false);
@@ -236,6 +249,9 @@ void GenericPlatformManagerImpl_FreeRTOS<ImplClass>::EventLoopTaskMain(void * ar
 {
     ChipLogDetail(DeviceLayer, "CHIP task running");
     static_cast<GenericPlatformManagerImpl_FreeRTOS<ImplClass> *>(arg)->Impl()->RunEventLoop();
+    // TODO: At this point, should we not
+    // vTaskDelete(static_cast<GenericPlatformManagerImpl_FreeRTOS<ImplClass> *>(arg)->mEventLoopTask)?
+    // Or somehow get our caller to do it once this thread is joined?
 }
 
 template <class ImplClass>
@@ -276,16 +292,6 @@ template <class ImplClass>
 void GenericPlatformManagerImpl_FreeRTOS<ImplClass>::_Shutdown(void)
 {
     GenericPlatformManagerImpl<ImplClass>::_Shutdown();
-
-    if (mChipEventQueue != NULL) {
-        vQueueDelete(mChipEventQueue);
-        mChipEventQueue = NULL;
-    }
-
-    if (mChipStackLock != NULL) {
-        vSemaphoreDelete(mChipStackLock);
-        mChipStackLock = NULL;
-    }
 }
 
 template <class ImplClass>