diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 5553cccdfbc611..c3105df31a16ec 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -31,6 +31,10 @@ jobs: strategy: matrix: type: [debug, tsan] + eventloop: [eventloop_same, eventloop_separate] + env: + USE_SEPARATE_EVENTLOOP: ${{ matrix.eventloop == 'eventloop_separate' }} + USE_TSAN: ${{ matrix.type == 'tsan' }} if: github.actor != 'restyled-io[bot]' runs-on: ubuntu-latest @@ -58,30 +62,18 @@ jobs: uses: actions/upload-artifact@v2 if: ${{ always() }} with: - name: bootstrap-logs-${{ matrix.type }} + name: bootstrap-logs-linux-${{ matrix.type }}-${{ matrix.eventloop }} path: | .environment/gn_out/.ninja_log .environment/pigweed-venv/*.log - name: Build all clusters app timeout-minutes: 5 run: | - case ${{ matrix.type }} in - "debug") GN_ARGS='is_tsan=false';; - "tsan") GN_ARGS='is_tsan=true';; - *) ;; - esac - - scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false "$GN_ARGS" + scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false is_tsan=${USE_TSAN} - name: Build chip-tool timeout-minutes: 5 run: | - case ${{ matrix.type }} in - "debug") GN_ARGS='is_tsan=false';; - "tsan") GN_ARGS='is_tsan=true';; - *) ;; - esac - - scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/ "$GN_ARGS" + scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/ is_tsan=${USE_TSAN} config_use_separate_eventloop=${USE_SEPARATE_EVENTLOOP} - name: Copy objdir run: | # The idea is to not upload our objdir unless builds have @@ -96,7 +88,7 @@ jobs: uses: actions/upload-artifact@v2 if: ${{ failure() }} with: - name: crash-core-linux-${{ matrix.type }} + name: crash-core-linux-${{ matrix.type }}-${{ matrix.eventloop }} path: /tmp/cores/ # Cores are big; don't hold on to them too long. retention-days: 5 @@ -104,7 +96,7 @@ jobs: uses: actions/upload-artifact@v2 if: ${{ failure() }} with: - name: crash-objdir-linux-${{ matrix.type }} + name: crash-objdir-linux-${{ matrix.type }}-${{ matrix.eventloop }} path: objdir-clone/ # objdirs are big; don't hold on to them too long. retention-days: 5 @@ -115,6 +107,10 @@ jobs: strategy: matrix: type: [debug, tsan] + eventloop: [eventloop_same, eventloop_separate] + env: + USE_SEPARATE_EVENTLOOP: ${{ matrix.eventloop == 'eventloop_separate' }} + USE_TSAN: ${{ matrix.type == 'tsan' }} if: github.actor != 'restyled-io[bot]' runs-on: macos-latest @@ -147,30 +143,18 @@ jobs: uses: actions/upload-artifact@v2 if: ${{ always() }} with: - name: bootstrap-logs-${{ matrix.type }} + name: bootstrap-logs-darwin-${{ matrix.type }}-${{ matrix.eventloop }} path: | .environment/gn_out/.ninja_log .environment/pigweed-venv/*.log - name: Run Build Test Server timeout-minutes: 5 run: | - case ${{ matrix.type }} in - "debug") GN_ARGS='is_tsan=false';; - "tsan") GN_ARGS='is_tsan=true';; - *) ;; - esac - - scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false "$GN_ARGS" + scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false is_tsan=${USE_TSAN} - name: Build chip-tool timeout-minutes: 5 run: | - case ${{ matrix.type }} in - "debug") GN_ARGS='is_tsan=false';; - "tsan") GN_ARGS='is_tsan=true';; - *) ;; - esac - - scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/ "$GN_ARGS" + scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/ is_tsan=${USE_TSAN} config_use_separate_eventloop=${USE_SEPARATE_EVENTLOOP} - name: Copy objdir run: | # The idea is to not upload our objdir unless builds have @@ -184,7 +168,7 @@ jobs: uses: actions/upload-artifact@v2 if: ${{ failure() }} with: - name: crash-core-darwin-${{ matrix.type }} + name: crash-core-darwin-${{ matrix.type }}-${{ matrix.eventloop }} path: /cores/ # Cores are big; don't hold on to them too long. retention-days: 5 @@ -192,13 +176,13 @@ jobs: uses: actions/upload-artifact@v2 if: ${{ failure() }} with: - name: crash-log-darwin-${{ matrix.type }} + name: crash-log-darwin-${{ matrix.type }}-${{ matrix.eventloop }} path: ~/Library/Logs/DiagnosticReports/ - name: Uploading objdir for debugging uses: actions/upload-artifact@v2 if: ${{ failure() }} with: - name: crash-objdir-darwin-${{ matrix.type }} + name: crash-objdir-darwin-${{ matrix.type }}-${{ matrix.eventloop }} path: objdir-clone/ # objdirs are big; don't hold on to them too long. retention-days: 5 diff --git a/examples/chip-tool/BUILD.gn b/examples/chip-tool/BUILD.gn index 09e7ede50edcb0..f1e5a2c9b9c8d8 100644 --- a/examples/chip-tool/BUILD.gn +++ b/examples/chip-tool/BUILD.gn @@ -19,6 +19,11 @@ import("${chip_root}/build/chip/tools.gni") assert(chip_build_tools) +declare_args() { + # Use a separate eventloop for CHIP tasks + config_use_separate_eventloop = true +} + executable("chip-tool") { sources = [ "commands/clusters/ModelCommand.cpp", @@ -36,6 +41,8 @@ executable("chip-tool") { "main.cpp", ] + defines = [ "CONFIG_USE_SEPARATE_EVENTLOOP=${config_use_separate_eventloop}" ] + deps = [ "${chip_root}/src/controller/data_model", "${chip_root}/src/lib", diff --git a/examples/chip-tool/commands/common/Command.cpp b/examples/chip-tool/commands/common/Command.cpp index 6012b065236a06..25ca0afad652f4 100644 --- a/examples/chip-tool/commands/common/Command.cpp +++ b/examples/chip-tool/commands/common/Command.cpp @@ -17,6 +17,7 @@ */ #include "Command.h" +#include "platform/PlatformManager.h" #include #include @@ -378,16 +379,30 @@ const char * Command::GetAttribute(void) const void Command::UpdateWaitForResponse(bool value) { +#if CONFIG_USE_SEPARATE_EVENTLOOP { std::lock_guard lk(cvWaitingForResponseMutex); mWaitingForResponse = value; } cvWaitingForResponse.notify_all(); +#else // CONFIG_USE_SEPARATE_EVENTLOOP + if (value == false) + { + if (mCommandExitStatus != CHIP_NO_ERROR) + { + ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(mCommandExitStatus)); + } + + chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); + } +#endif // CONFIG_USE_SEPARATE_EVENTLOOP } -void Command::WaitForResponse(uint16_t duration) +#if CONFIG_USE_SEPARATE_EVENTLOOP + +void Command::WaitForResponse(uint16_t seconds) { - std::chrono::seconds waitingForResponseTimeout(duration); + std::chrono::seconds waitingForResponseTimeout(seconds); std::unique_lock lk(cvWaitingForResponseMutex); auto waitingUntil = std::chrono::system_clock::now() + waitingForResponseTimeout; if (!cvWaitingForResponse.wait_until(lk, waitingUntil, [this]() { return !this->mWaitingForResponse; })) @@ -395,3 +410,30 @@ void Command::WaitForResponse(uint16_t duration) ChipLogError(chipTool, "No response from device"); } } + +#else // CONFIG_USE_SEPARATE_EVENTLOOP + +static void OnResponseTimeout(chip::System::Layer *, void *, chip::System::Error) +{ + ChipLogError(chipTool, "No response from device"); + + chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); +} + +CHIP_ERROR Command::ScheduleWaitForResponse(uint16_t seconds) +{ + chip::System::Timer * timer = nullptr; + + CHIP_ERROR err = chip::DeviceLayer::SystemLayer.NewTimer(timer); + if (err == CHIP_NO_ERROR) + { + timer->Start(seconds * 1000, OnResponseTimeout, this); + } + else + { + ChipLogError(chipTool, "Failed to allocate timer"); + } + return err; +} + +#endif // CONFIG_USE_SEPARATE_EVENTLOOP diff --git a/examples/chip-tool/commands/common/Command.h b/examples/chip-tool/commands/common/Command.h index dc4bf9d3c5e8fa..15d5385d848661 100644 --- a/examples/chip-tool/commands/common/Command.h +++ b/examples/chip-tool/commands/common/Command.h @@ -187,7 +187,18 @@ class Command } void UpdateWaitForResponse(bool value); - void WaitForResponse(uint16_t duration); + + // There is a certain symmetry between the single-event-loop and + // separate-event-loop approaches. With a separate event loop, we schedule + // our work on that event loop and synchronously wait (block) waiting for a + // response. When using a single event loop, we ask for an async response + // notification and then block processing work on the event loop + // synchronously until that notification happens. +#if CONFIG_USE_SEPARATE_EVENTLOOP + void WaitForResponse(uint16_t seconds); +#else // CONFIG_USE_SEPARATE_EVENTLOOP + CHIP_ERROR ScheduleWaitForResponse(uint16_t seconds); +#endif // CONFIG_USE_SEPARATE_EVENTLOOP protected: ExecutionContext * GetExecContext() { return mExecContext; } @@ -202,7 +213,9 @@ class Command const char * mName = nullptr; std::vector mArgs; +#if CONFIG_USE_SEPARATE_EVENTLOOP std::condition_variable cvWaitingForResponse; std::mutex cvWaitingForResponseMutex; bool mWaitingForResponse{ false }; +#endif // CONFIG_USE_SEPARATE_EVENTLOOP }; diff --git a/examples/chip-tool/commands/common/Commands.cpp b/examples/chip-tool/commands/common/Commands.cpp index 27ebd2400f3c23..b8cca51a9a6aad 100644 --- a/examples/chip-tool/commands/common/Commands.cpp +++ b/examples/chip-tool/commands/common/Commands.cpp @@ -70,16 +70,34 @@ int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv) err = mController.Init(localId, initParams); VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", chip::ErrorStr(err))); +#if CONFIG_USE_SEPARATE_EVENTLOOP + // ServiceEvents() calls StartEventLoopTask(), which is paired with the + // StopEventLoopTask() below. err = mController.ServiceEvents(); VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", chip::ErrorStr(err))); +#endif // CONFIG_USE_SEPARATE_EVENTLOOP err = RunCommand(localId, remoteId, argc, argv, &command); SuccessOrExit(err); +#if !CONFIG_USE_SEPARATE_EVENTLOOP + chip::DeviceLayer::PlatformMgr().RunEventLoop(); +#endif // !CONFIG_USE_SEPARATE_EVENTLOOP + + if (command) + { + err = command->GetCommandExitStatus(); + } + exit: -#if CONFIG_DEVICE_LAYER + if (err != CHIP_NO_ERROR) + { + ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err)); + } + +#if CONFIG_USE_SEPARATE_EVENTLOOP chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); -#endif +#endif // CONFIG_USE_SEPARATE_EVENTLOOP if (command) { @@ -176,14 +194,14 @@ CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char // set the variable to true, which will cause it to block indefinitely. // command->UpdateWaitForResponse(true); +#if CONFIG_USE_SEPARATE_EVENTLOOP chip::DeviceLayer::PlatformMgr().ScheduleWork(RunQueuedCommand, reinterpret_cast(command)); command->WaitForResponse(command->GetWaitDurationInSeconds()); - err = command->GetCommandExitStatus(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err)); - ExitNow(); - } +#else // CONFIG_USE_SEPARATE_EVENTLOOP + err = command->Run(); + SuccessOrExit(err); + command->ScheduleWaitForResponse(command->GetWaitDurationInSeconds()); +#endif // CONFIG_USE_SEPARATE_EVENTLOOP } exit: diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp index 0fe3dc3d7a953c..c59f020392bbc3 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp +++ b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp @@ -298,8 +298,6 @@ CHIP_ERROR GenericPlatformManagerImpl_POSIX::_StopEventLoopTask() } exit: - pthread_mutex_destroy(&mStateLock); - pthread_cond_destroy(&mEventQueueStoppedCond); mHasValidChipTask = false; return System::MapErrorPOSIX(err); } @@ -307,6 +305,9 @@ CHIP_ERROR GenericPlatformManagerImpl_POSIX::_StopEventLoopTask() template CHIP_ERROR GenericPlatformManagerImpl_POSIX::_Shutdown() { + pthread_mutex_destroy(&mStateLock); + pthread_cond_destroy(&mEventQueueStoppedCond); + // // Call up to the base class _Shutdown() to perform the actual stack de-initialization // and clean-up