From 0322eb1742874a50934f6030fc8341f5e5a8fda2 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Thu, 17 Jun 2021 23:51:34 -0700 Subject: [PATCH] Allow post event from any thread to CHIP main event loop. (#7195) * Make access to CHIP Message Queue thread safe * Use RAII style of locking * Add support for thread-safe event queue in platform layer * Refactor Front() and Pop() to PopFront() --- .../GenericPlatformManagerImpl_POSIX.cpp | 10 +-- .../GenericPlatformManagerImpl_POSIX.h | 3 +- src/platform/BUILD.gn | 4 ++ src/platform/DeviceSafeQueue.cpp | 55 +++++++++++++++ src/platform/DeviceSafeQueue.h | 67 +++++++++++++++++++ src/platform/Linux/PlatformManagerImpl.cpp | 2 - 6 files changed, 133 insertions(+), 8 deletions(-) create mode 100644 src/platform/DeviceSafeQueue.cpp create mode 100644 src/platform/DeviceSafeQueue.h diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp index 81d7584cdc6f26..88ae6a4e6f7f06 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp +++ b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp @@ -147,17 +147,17 @@ CHIP_ERROR GenericPlatformManagerImpl_POSIX::_StartChipTimer(int64_t template void GenericPlatformManagerImpl_POSIX::_PostEvent(const ChipDeviceEvent * event) { - mChipEventQueue.push(*event); // Thread safe due to ChipStackLock taken by App thread - SysOnEventSignal(this); // Trigger wake select on CHIP thread + mChipEventQueue.Push(*event); + SysOnEventSignal(this); // Trigger wake select on CHIP thread } template void GenericPlatformManagerImpl_POSIX::ProcessDeviceEvents() { - while (!mChipEventQueue.empty()) + while (!mChipEventQueue.Empty()) { - Impl()->DispatchEvent(&mChipEventQueue.front()); - mChipEventQueue.pop(); + const ChipDeviceEvent event = mChipEventQueue.PopFront(); + Impl()->DispatchEvent(&event); } } diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h index 7d15d505cba86f..5c498cc1f6a2f9 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h +++ b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h @@ -24,6 +24,7 @@ #pragma once +#include #include #include @@ -61,7 +62,6 @@ class GenericPlatformManagerImpl_POSIX : public GenericPlatformManagerImpl mChipEventQueue; enum TaskType { @@ -121,6 +121,7 @@ class GenericPlatformManagerImpl_POSIX : public GenericPlatformManagerImpl mShouldRunEventLoop; static void * EventLoopTaskMain(void * arg); }; diff --git a/src/platform/BUILD.gn b/src/platform/BUILD.gn index 6b201393e977a3..1de3fd300ac259 100644 --- a/src/platform/BUILD.gn +++ b/src/platform/BUILD.gn @@ -350,6 +350,8 @@ if (chip_device_platform != "none" && chip_device_platform != "external") { "Darwin/PosixConfig.cpp", "Darwin/PosixConfig.h", "Darwin/SystemPlatformConfig.h", + "DeviceSafeQueue.cpp", + "DeviceSafeQueue.h", ] if (chip_enable_ble) { @@ -487,6 +489,8 @@ if (chip_device_platform != "none" && chip_device_platform != "external") { public_deps += [ "${chip_root}/src/crypto" ] } else if (chip_device_platform == "linux") { sources += [ + "DeviceSafeQueue.cpp", + "DeviceSafeQueue.h", "Linux/BLEManagerImpl.cpp", "Linux/BLEManagerImpl.h", "Linux/BlePlatformConfig.h", diff --git a/src/platform/DeviceSafeQueue.cpp b/src/platform/DeviceSafeQueue.cpp new file mode 100644 index 00000000000000..ea12becd026918 --- /dev/null +++ b/src/platform/DeviceSafeQueue.cpp @@ -0,0 +1,55 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * This file defines the CHIP device event queue which operates in a FIFO context (first-in first-out), + * and provides a specific set of member functions to access its elements wth thread safety. + */ + +#include + +namespace chip { +namespace DeviceLayer { +namespace Internal { + +void DeviceSafeQueue::Push(const ChipDeviceEvent & event) +{ + std::unique_lock lock(mEventQueueLock); + mEventQueue.push(event); +} + +bool DeviceSafeQueue::Empty() +{ + std::unique_lock lock(mEventQueueLock); + return mEventQueue.empty(); +} + +const ChipDeviceEvent DeviceSafeQueue::PopFront() +{ + std::unique_lock lock(mEventQueueLock); + + const ChipDeviceEvent event = mEventQueue.front(); + mEventQueue.pop(); + + return event; +} + +} // namespace Internal +} // namespace DeviceLayer +} // namespace chip diff --git a/src/platform/DeviceSafeQueue.h b/src/platform/DeviceSafeQueue.h new file mode 100644 index 00000000000000..3d53ec1bca8675 --- /dev/null +++ b/src/platform/DeviceSafeQueue.h @@ -0,0 +1,67 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * This file declares the CHIP device event queue which operates in a FIFO context (first-in first-out), + * and provides a specific set of member functions to access its elements wth thread safety. + */ + +#pragma once + +#include +#include + +#include +#include +#include + +namespace chip { +namespace DeviceLayer { +namespace Internal { + +/** + * @class DeviceSafeQueue + * + * @brief + * This class represents a thread-safe message queue implemented with C++ Standard Library, the message queue + * is used by the CHIP event loop to hold incoming messages. Each message is sequentially dequeued, decoded, + * and then an action is performed. + * + */ +class DeviceSafeQueue +{ +public: + DeviceSafeQueue() = default; + ~DeviceSafeQueue() = default; + + void Push(const ChipDeviceEvent & event); + bool Empty(); + const ChipDeviceEvent PopFront(); + +private: + std::queue mEventQueue; + std::mutex mEventQueueLock; + + DeviceSafeQueue(const DeviceSafeQueue &) = delete; + DeviceSafeQueue & operator=(const DeviceSafeQueue &) = delete; +}; + +} // namespace Internal +} // namespace DeviceLayer +} // namespace chip diff --git a/src/platform/Linux/PlatformManagerImpl.cpp b/src/platform/Linux/PlatformManagerImpl.cpp index b45e6d01475c33..ef99319b811f61 100644 --- a/src/platform/Linux/PlatformManagerImpl.cpp +++ b/src/platform/Linux/PlatformManagerImpl.cpp @@ -107,9 +107,7 @@ void PlatformManagerImpl::WiFIIPChangeListener() ChipLogDetail(DeviceLayer, "Got IP address on interface: %s IP: %s", name, event.InternetConnectivityChange.address); - PlatformMgr().LockChipStack(); PlatformMgr().PostEvent(&event); - PlatformMgr().UnlockChipStack(); } routeInfo = RTA_NEXT(routeInfo, rtl); }