From 116774426eb05043b7b0c2ffcb85455be6a8b7d9 Mon Sep 17 00:00:00 2001 From: Artur Tynecki <77382963+ATmobica@users.noreply.github.com> Date: Fri, 16 Jun 2023 21:32:15 +0200 Subject: [PATCH] [OIS] Platform improvements (#27291) * [OIS] Improve time to ticks conversion functions Signed-off-by: ATmobica * [OIS] Improve PlatformManagerImpl Use an atomic variable to control the event loop running. Improve stack init - enable reinitialization. Improve stack locking and evnet loop termination. Signed-off-by: ATmobica --------- Signed-off-by: ATmobica --- src/platform/openiotsdk/OpenIoTSDKArchUtils.c | 15 - .../openiotsdk/PlatformManagerImpl.cpp | 266 ++++++++++-------- src/platform/openiotsdk/PlatformManagerImpl.h | 15 +- 3 files changed, 163 insertions(+), 133 deletions(-) diff --git a/src/platform/openiotsdk/OpenIoTSDKArchUtils.c b/src/platform/openiotsdk/OpenIoTSDKArchUtils.c index d7c74ac5a21cae..96378c7f24f730 100644 --- a/src/platform/openiotsdk/OpenIoTSDKArchUtils.c +++ b/src/platform/openiotsdk/OpenIoTSDKArchUtils.c @@ -46,31 +46,16 @@ uint64_t GetTick(void) /* Time to kernel ticks */ uint32_t sec2tick(uint32_t sec) { - if (sec == 0U) - { - return (osWaitForever); - } - return (sec * osKernelGetTickFreq()); } uint32_t ms2tick(uint32_t ms) { - if (ms == 0U) - { - return (osWaitForever); - } - return (uint32_t)(((uint64_t) ms * (uint64_t) osKernelGetTickFreq()) / 1000U); } uint32_t us2tick(uint32_t usec) { - if (usec == 0U) - { - return osWaitForever; - } - // round division up because our tick is so long this might become 0 // we need the timer to sleep at least one tick as it otherwise breaks expectations return (uint32_t)(((uint64_t) usec * osKernelGetTickFreq() + (1000000 - 1)) / 1000000); diff --git a/src/platform/openiotsdk/PlatformManagerImpl.cpp b/src/platform/openiotsdk/PlatformManagerImpl.cpp index 579eb5e7f10429..896617d781e31f 100644 --- a/src/platform/openiotsdk/PlatformManagerImpl.cpp +++ b/src/platform/openiotsdk/PlatformManagerImpl.cpp @@ -34,48 +34,78 @@ using namespace ::chip; using namespace ::chip::Inet; using namespace ::chip::System; +using namespace chip::System::Clock; namespace chip { namespace DeviceLayer { +struct ScopedLock +{ + ScopedLock(osMutexId_t & lockable) : _lockable(lockable) { osMutexAcquire(_lockable, osWaitForever); } + ScopedLock(const ScopedLock &) = delete; + ScopedLock & operator=(const ScopedLock &) = delete; + ~ScopedLock() { osMutexRelease(_lockable); } + +private: + osMutexId_t & _lockable; +}; + +namespace { +LayerImpl & SystemLayerImpl() +{ + return static_cast(DeviceLayer::SystemLayer()); +} +} // anonymous namespace + CHIP_ERROR PlatformManagerImpl::_InitChipStack(void) { - if (mInitialized) + // Members are initialized by the stack + osMutexAttr_t mut_att = { .attr_bits = osMutexRecursive }; + + // Reinitialize the Mutexes + if (mChipStackMutex == nullptr) { - return CHIP_NO_ERROR; + mChipStackMutex = osMutexNew(&mut_att); } - // Call up to the base class _InitChipStack() to perform the bulk of the initialization. - CHIP_ERROR err = GenericPlatformManagerImpl::_InitChipStack(); - if (err != CHIP_NO_ERROR) + if (mEventTaskMutex == nullptr) { - return err; + mEventTaskMutex = osMutexNew(nullptr); } - // Members are initialized by the stack - osMutexAttr_t mut_att = { .attr_bits = osMutexRecursive }; + if (mPlatformFlags == nullptr) + { + mPlatformFlags = osEventFlagsNew(nullptr); + } - // Reinitialize the Mutexes - mChipStackMutex = osMutexNew(&mut_att); - mEventTaskMutex = osMutexNew(nullptr); - mPlatformFlags = osEventFlagsNew(nullptr); - mQueue = osMessageQueueNew(CHIP_DEVICE_CONFIG_MAX_EVENT_QUEUE_SIZE, sizeof(ChipDeviceEvent), nullptr); - mTimer = osTimerNew(TimerCallback, osTimerOnce, NULL, NULL); + if (mQueue == nullptr) + { + mQueue = osMessageQueueNew(CHIP_DEVICE_CONFIG_MAX_EVENT_QUEUE_SIZE, sizeof(ChipDeviceEvent), nullptr); + } + else + { + osMessageQueueReset(mQueue); + } - if (!mChipStackMutex || !mEventTaskMutex || !mPlatformFlags || !mQueue || !mTimer) + if (!mChipStackMutex || !mEventTaskMutex || !mPlatformFlags || !mQueue) { osMutexDelete(mChipStackMutex); osMutexDelete(mEventTaskMutex); osEventFlagsDelete(mPlatformFlags); osMessageQueueDelete(mQueue); - osTimerDelete(mTimer); - mChipStackMutex = mEventTaskMutex = mPlatformFlags = mQueue = mTimer = nullptr; + mChipStackMutex = mEventTaskMutex = mPlatformFlags = mQueue = nullptr; return CHIP_ERROR_INTERNAL; } + ReturnLogErrorOnFailure(PlatformTimerInit()); + + mRunEventLoop.store(false); mInitialized = true; + // Call up to the base class _InitChipStack() to perform the bulk of the initialization. + ReturnLogErrorOnFailure(GenericPlatformManagerImpl::_InitChipStack()); + SetConfigurationMgr(&ConfigurationManagerImpl::GetDefaultInstance()); SetDiagnosticDataProvider(&DiagnosticDataProviderImpl::GetDefaultInstance()); @@ -106,13 +136,15 @@ void PlatformManagerImpl::_UnlockChipStack() CHIP_ERROR PlatformManagerImpl::_PostEvent(const ChipDeviceEvent * eventPtr) { - // The post event requires event queue from stack initialization - ReturnLogErrorOnFailure(_InitChipStack()); + if (!mInitialized) + { + ChipLogError(DeviceLayer, "_PostEvent: stack not initialized"); + return CHIP_ERROR_INCORRECT_STATE; + } osStatus_t status = osMessageQueuePut(mQueue, eventPtr, 0, 0); - CHIP_ERROR ret = (status == osOK) ? CHIP_NO_ERROR : CHIP_ERROR_INTERNAL; osEventFlagsSet(mPlatformFlags, kPostEventFlag); - return ret; + return (status == osOK) ? CHIP_NO_ERROR : CHIP_ERROR_INTERNAL; } void PlatformManagerImpl::HandlePostEvent() @@ -127,79 +159,88 @@ void PlatformManagerImpl::HandlePostEvent() break; } - LockChipStack(); DispatchEvent(&event); - UnlockChipStack(); count--; } } void PlatformManagerImpl::HandleTimerEvent(void) { - const CHIP_ERROR err = static_cast(DeviceLayer::SystemLayer()).HandlePlatformTimer(); + CHIP_ERROR err = SystemLayerImpl().HandlePlatformTimer(); if (err != CHIP_NO_ERROR) { ChipLogError(DeviceLayer, "HandlePlatformTimer %ld", err.AsInteger()); } } -void PlatformManagerImpl::RunEventLoopInternal() +void PlatformManagerImpl::_RunEventLoop() { uint32_t flags = 0; - while (true) + + if (!mInitialized) + { + ChipLogError(DeviceLayer, "_PostEvent: stack not initialized"); + return; + } + { - flags = osEventFlagsWait(mPlatformFlags, kPostEventFlag | kTimerEventFlag | kTaskStopEventFlag, - osFlagsWaitAny | osFlagsNoClear, ms2tick(1000)); + ScopedLock lock(mEventTaskMutex); - // in case of error we still need to know the value of flags we're not waiting for - if (flags & osFlagsError) + bool expectedValue = false; + if (!mRunEventLoop.compare_exchange_strong(expectedValue /* expected */, true /* desired */)) { - flags = osEventFlagsGet(mPlatformFlags); + ChipLogError(DeviceLayer, "Error trying to run the event loop while it is already running"); + return; } - if (flags & kTaskStopEventFlag) + // Look if a task ID has already been assigned or not. + // If not, it means we run in the thread that called RunEventLoop + if (!mEventTask) { - osEventFlagsClear(mPlatformFlags, kTaskStopEventFlag); - osEventFlagsClear(mPlatformFlags, kTaskRunningEventFlag); - break; + ChipLogDetail(DeviceLayer, "Run CHIP event loop on external thread"); + mEventTask = osThreadGetId(); + } + else + { + osEventFlagsSet(mPlatformFlags, kTaskHasEventLoopRunFlag); + } + } + + LockChipStack(); + + while (mRunEventLoop.load()) + { + UnlockChipStack(); + flags = osEventFlagsWait(mPlatformFlags, kPostEventFlag | kTimerEventFlag, osFlagsWaitAny, osWaitForever); + LockChipStack(); + + // In case of error we still need to know the value of flags we're not waiting for + if (flags & osFlagsError) + { + flags = osEventFlagsGet(mPlatformFlags); } if (flags & kTimerEventFlag) { - osEventFlagsClear(mPlatformFlags, kTimerEventFlag); HandleTimerEvent(); } if (flags & kPostEventFlag) { - osEventFlagsClear(mPlatformFlags, kPostEventFlag); HandlePostEvent(); } - - if ((flags & kTaskRunningEventFlag) == 0) - { - break; - } - } -} - -void PlatformManagerImpl::_RunEventLoop() -{ - if (!mInitialized) - { - ChipLogError(DeviceLayer, "_RunEventLoop: stack not initialized"); - return; } - osEventFlagsSet(mPlatformFlags, kTaskRunningEventFlag); + UnlockChipStack(); - RunEventLoopInternal(); + osEventFlagsSet(mPlatformFlags, kTaskHasEventLoopStopFlag); + mEventTask = nullptr; } void PlatformManagerImpl::EventLoopTask(void * arg) { (void) arg; - PlatformMgrImpl().RunEventLoopInternal(); + PlatformMgrImpl().RunEventLoop(); osThreadTerminate(osThreadGetId()); } @@ -211,15 +252,8 @@ CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask() return CHIP_ERROR_INCORRECT_STATE; } - // this mutex only needed to guard against multiple launches { - osMutexAcquire(mEventTaskMutex, osWaitForever); - - if (kTaskRunningEventFlag & osEventFlagsGet(mPlatformFlags)) - { - osMutexRelease(mEventTaskMutex); - return CHIP_ERROR_BUSY; - } + ScopedLock lock(mEventTaskMutex); const osThreadAttr_t tread_attr = { .name = CHIP_DEVICE_CONFIG_CHIP_TASK_NAME, @@ -228,18 +262,18 @@ CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask() }; - osEventFlagsSet(mPlatformFlags, kTaskRunningEventFlag); - - // this thread is self terminating - osThreadId_t mEventTask = osThreadNew(EventLoopTask, NULL, &tread_attr); - + mEventTask = osThreadNew(EventLoopTask, NULL, &tread_attr); if (mEventTask == nullptr) { - osMutexRelease(mEventTaskMutex); + ChipLogError(DeviceLayer, "Create event loop thread failed"); return CHIP_ERROR_INTERNAL; } + } - osMutexRelease(mEventTaskMutex); + if (osEventFlagsWait(mPlatformFlags, kTaskHasEventLoopRunFlag, osFlagsWaitAny, osWaitForever) & osFlagsError) + { + ChipLogError(DeviceLayer, "Start event loop thread failed"); + return CHIP_ERROR_INTERNAL; } return CHIP_NO_ERROR; @@ -247,32 +281,30 @@ CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask() CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask() { - if (!mInitialized) - { - ChipLogError(DeviceLayer, "_StopEventLoopTask: stack not initialized"); - return CHIP_ERROR_INCORRECT_STATE; - } - - // this mutex only needed to guard against multiple calls to stop { - osMutexAcquire(mEventTaskMutex, osWaitForever); + ScopedLock lock(mEventTaskMutex); - uint32_t flags = osEventFlagsGet(mPlatformFlags); - if ((kTaskRunningEventFlag & flags) == 0) + // Early return if the event loop is not running + if (!mRunEventLoop.load()) { - osMutexRelease(mEventTaskMutex); - return CHIP_ERROR_INCORRECT_STATE; + return CHIP_NO_ERROR; } - if (kTaskStopEventFlag & flags) - { - osMutexRelease(mEventTaskMutex); - return CHIP_ERROR_INCORRECT_STATE; - } + // Indicate that the event loop store + mRunEventLoop.store(false); + } - osEventFlagsSet(mPlatformFlags, kTaskStopEventFlag); + osEventFlagsSet(mPlatformFlags, kPostEventFlag); - osMutexRelease(mEventTaskMutex); + // If the thread running the event loop is different from the caller + // then wait it to finish + if (mEventTask != nullptr && mEventTask != osThreadGetId()) + { + if (osEventFlagsWait(mPlatformFlags, kTaskHasEventLoopStopFlag, osFlagsWaitAny, ms2tick(1000)) & osFlagsError) + { + ChipLogError(DeviceLayer, "Stop event loop thread failed"); + return CHIP_ERROR_INTERNAL; + } } return CHIP_NO_ERROR; @@ -288,63 +320,73 @@ void PlatformManagerImpl::TimerCallback(void * arg) PlatformMgrImpl().SetEventFlags(kTimerEventFlag); } -CHIP_ERROR PlatformManagerImpl::_StartChipTimer(System::Clock::Timeout duration) +CHIP_ERROR PlatformManagerImpl::PlatformTimerInit() { - // The timer requires event queue from stack initialization - ReturnLogErrorOnFailure(_InitChipStack()); + if (mTimer != nullptr) + { + return CHIP_NO_ERROR; + } + + mTimer = osTimerNew(TimerCallback, osTimerOnce, NULL, NULL); + if (!mTimer) + { + return CHIP_ERROR_INTERNAL; + } + + return CHIP_NO_ERROR; +} + +CHIP_ERROR PlatformManagerImpl::PlatformTimerDeinit() +{ + if (mTimer == nullptr) + { + return CHIP_NO_ERROR; + } + + osTimerDelete(mTimer); + mTimer = nullptr; + + return CHIP_NO_ERROR; +} + +CHIP_ERROR PlatformManagerImpl::_StartChipTimer(System::Clock::Timeout duration) +{ if (duration.count() == 0) { TimerCallback(0); } else { - auto res = osTimerStart(mTimer, ms2tick(duration.count())); + Milliseconds32 msec = std::chrono::duration_cast(duration); + auto res = osTimerStart(mTimer, ms2tick(msec.count())); if (res) { ChipLogError(DeviceLayer, "osTimerStart failed %d", res); return CHIP_ERROR_INTERNAL; } } + return CHIP_NO_ERROR; } void PlatformManagerImpl::_Shutdown() { - if (!mInitialized) - { - ChipLogError(DeviceLayer, "_Shutdown: stack not initialized"); - return; - } - // // Call up to the base class _Shutdown() to perform the actual stack de-initialization // and clean-up // - (void) _StopEventLoopTask(); - // the task thread is self terminating, we might have to wait if it's still processing - while (true) - { - uint32_t flags = osEventFlagsGet(mPlatformFlags); - if ((kTaskRunningEventFlag & flags) == 0) - { - break; - } - osDelay(1); - } - osMutexDelete(mChipStackMutex); osMutexDelete(mEventTaskMutex); osEventFlagsDelete(mPlatformFlags); osMessageQueueDelete(mQueue); - osTimerDelete(mTimer); + PlatformTimerDeinit(); mChipStackMutex = nullptr; mPlatformFlags = nullptr; mEventTaskMutex = nullptr; mQueue = nullptr; - mTimer = nullptr; mInitialized = false; GenericPlatformManagerImpl::_Shutdown(); diff --git a/src/platform/openiotsdk/PlatformManagerImpl.h b/src/platform/openiotsdk/PlatformManagerImpl.h index 2a5e86e8341e5d..cc7637d0f1ca54 100644 --- a/src/platform/openiotsdk/PlatformManagerImpl.h +++ b/src/platform/openiotsdk/PlatformManagerImpl.h @@ -26,6 +26,7 @@ #include "cmsis_os2.h" #include "mbedtls/platform.h" +#include #include #include #include @@ -53,10 +54,10 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener /* none so far */ private: - static constexpr uint32_t kTaskRunningEventFlag = 1 << 0; - static constexpr uint32_t kTaskStopEventFlag = 1 << 1; - static constexpr uint32_t kPostEventFlag = 1 << 2; - static constexpr uint32_t kTimerEventFlag = 1 << 3; + static constexpr uint32_t kTaskHasEventLoopRunFlag = 1 << 0; + static constexpr uint32_t kTaskHasEventLoopStopFlag = 1 << 1; + static constexpr uint32_t kPostEventFlag = 1 << 2; + static constexpr uint32_t kTimerEventFlag = 1 << 3; // ===== Methods that implement the PlatformManager abstract interface. @@ -71,6 +72,8 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener CHIP_ERROR _StartEventLoopTask(); CHIP_ERROR _StopEventLoopTask(); + CHIP_ERROR PlatformTimerInit(void); + CHIP_ERROR PlatformTimerDeinit(void); CHIP_ERROR _StartChipTimer(System::Clock::Timeout duration); void _Shutdown(); @@ -81,8 +84,6 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener static void EventLoopTask(void * arg); static void TimerCallback(void * arg); - void RunEventLoopInternal(void); - // ===== Members for internal use by the following friends. friend PlatformManager & PlatformMgr(void); @@ -101,6 +102,8 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener osMessageQueueId_t mQueue = nullptr; osTimerId_t mTimer = nullptr; bool mInitialized = false; + std::atomic mRunEventLoop; + osThreadId_t mEventTask = nullptr; }; /**