From 8c2b5aa7b08f1bbd6d8497e0df3860cfba790d86 Mon Sep 17 00:00:00 2001 From: Dawid Wojtas Date: Mon, 13 Feb 2023 17:03:42 +0100 Subject: [PATCH] [BH-1624] Fix shutdown procedure In some cases, the system wasn't able to turn off because the GUI service got stuck. The device was still working in the background. The cause was an empty queue in DrawCommandQueue which hang the GUI worker. The interface was modified and synchronization mechanism was removed. The thread no longer waits in dequeue(). Also changed the worker to close in the right way the logger worker. --- .../service-gui/DrawCommandsQueue.cpp | 26 +++++++++---------- .../service-gui/DrawCommandsQueue.hpp | 13 ++++------ module-services/service-gui/WorkerGUI.cpp | 10 +++---- .../tests/test-DrawCommandsQueue.cpp | 15 +++++------ module-sys/Service/Worker.cpp | 23 +++++----------- module-sys/Service/include/Service/Worker.hpp | 3 +-- module-utils/log/Logger.cpp | 21 ++++++++++----- module-utils/log/Logger.hpp | 4 +-- 8 files changed, 52 insertions(+), 63 deletions(-) diff --git a/module-services/service-gui/DrawCommandsQueue.cpp b/module-services/service-gui/DrawCommandsQueue.cpp index a9a9cfe16ab03cce49a687bead948fe38dccc84f..d43e1ba0bf46d96fa3670fe8a6636003009cf401 100644 --- a/module-services/service-gui/DrawCommandsQueue.cpp +++ b/module-services/service-gui/DrawCommandsQueue.cpp @@ -1,18 +1,16 @@ -// Copyright (c) 2017-2021, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #include "DrawCommandsQueue.hpp" +#include + +#include + namespace service::gui { - namespace - { - constexpr std::chrono::milliseconds WaitTimeout{5000}; - } // namespace - DrawCommandsQueue::DrawCommandsQueue(std::size_t expectedSize, - std::unique_ptr &&synchronization) - : synchronization{std::move(synchronization)} + DrawCommandsQueue::DrawCommandsQueue(std::size_t expectedSize) { queue.reserve(expectedSize); } @@ -21,16 +19,16 @@ namespace service::gui { cpp_freertos::LockGuard lock{queueMutex}; queue.push_back(std::move(item)); - - synchronization->notify(); } - auto DrawCommandsQueue::dequeue() -> QueueItem + auto DrawCommandsQueue::dequeue() -> std::optional { + std::optional item = std::nullopt; cpp_freertos::LockGuard lock{queueMutex}; - synchronization->wait(queueMutex, WaitTimeout, [this]() { return !queue.empty(); }); - - auto item = std::move(queue.front()); + if (queue.empty()) { + return item; + } + item = std::move(queue.front()); queue.erase(queue.begin()); return item; } diff --git a/module-services/service-gui/DrawCommandsQueue.hpp b/module-services/service-gui/DrawCommandsQueue.hpp index f444514f9ceba77d9059e382e7dcffebd313e78e..ddb6e551d9b336d32fc3d43cf9f15dd2603c6ef7 100644 --- a/module-services/service-gui/DrawCommandsQueue.hpp +++ b/module-services/service-gui/DrawCommandsQueue.hpp @@ -1,11 +1,10 @@ -// Copyright (c) 2017-2021, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once -#include "SynchronizationMechanism.hpp" - #include +#include #include #include @@ -25,12 +24,11 @@ namespace service::gui }; using QueueContainer = std::vector; - explicit DrawCommandsQueue( - std::size_t expectedSize, - std::unique_ptr &&synchronization = getFreeRtosSynchronizationMechanism()); + explicit DrawCommandsQueue(std::size_t expectedSize); void enqueue(QueueItem &&item); - [[nodiscard]] auto dequeue() -> QueueItem; + auto stop() -> void; + [[nodiscard]] auto dequeue() -> std::optional; [[nodiscard]] auto getMaxRefreshModeAndClear() -> ::gui::RefreshModes; void clear(); [[nodiscard]] auto size() const noexcept -> QueueContainer::size_type; @@ -39,6 +37,5 @@ namespace service::gui QueueContainer queue; mutable cpp_freertos::MutexStandard queueMutex; - std::unique_ptr synchronization; }; } // namespace service::gui diff --git a/module-services/service-gui/WorkerGUI.cpp b/module-services/service-gui/WorkerGUI.cpp index ba565e9b5f3a56d45ee35b543ce8fe50065a4aa5..abdc950f69544924db86c2adba5c26cb139c5041 100644 --- a/module-services/service-gui/WorkerGUI.cpp +++ b/module-services/service-gui/WorkerGUI.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #include "WorkerGUI.hpp" @@ -39,7 +39,9 @@ namespace service::gui switch (command) { case Signal::Render: { auto item = guiService->commandsQueue->dequeue(); - render(item.commands, item.refreshMode); + if (item.has_value()) { + render(item->commands, item->refreshMode); + } break; } case Signal::ChangeColorScheme: { @@ -54,13 +56,10 @@ namespace service::gui void WorkerGUI::render(DrawCommandsQueue::CommandList &commands, ::gui::RefreshModes refreshMode) { const auto [contextId, context] = guiService->contextPool->borrowContext(); // Waits for the context. - renderer.render(context, commands); - #if DEBUG_EINK_REFRESH == 1 LOG_INFO("Render ContextId: %d\n%s", contextId, context->toAsciiScaled().c_str()); #endif - onRenderingFinished(contextId, refreshMode); } @@ -74,4 +73,5 @@ namespace service::gui auto msg = std::make_shared(contextId, refreshMode); guiService->bus.sendUnicast(std::move(msg), guiService->GetName()); } + } // namespace service::gui diff --git a/module-services/service-gui/tests/test-DrawCommandsQueue.cpp b/module-services/service-gui/tests/test-DrawCommandsQueue.cpp index ed748088d4e5608be2b58d7f4f5b7cf4d8b59a41..770b9a766f63d1b85a558e691a300aeb425d0ad5 100644 --- a/module-services/service-gui/tests/test-DrawCommandsQueue.cpp +++ b/module-services/service-gui/tests/test-DrawCommandsQueue.cpp @@ -1,10 +1,9 @@ -// Copyright (c) 2017-2021, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #include #include "DrawCommandsQueue.hpp" -#include "MockedSynchronizationMechanism.hpp" #include #include @@ -14,7 +13,7 @@ using namespace service::gui; TEST_CASE("DrawCommandsQueueTests") { constexpr auto ExpectedQueueSize = 4U; - DrawCommandsQueue queue{ExpectedQueueSize, std::make_unique()}; + DrawCommandsQueue queue{ExpectedQueueSize}; SECTION("Enqueue") { @@ -28,25 +27,25 @@ TEST_CASE("DrawCommandsQueueTests") REQUIRE(queue.size() == 1U); const auto item = queue.dequeue(); - REQUIRE(item.commands.empty()); - REQUIRE(item.refreshMode == ::gui::RefreshModes::GUI_REFRESH_FAST); + REQUIRE(item->commands.empty()); + REQUIRE(item->refreshMode == ::gui::RefreshModes::GUI_REFRESH_FAST); REQUIRE(queue.size() == 0); } SECTION("Multi-thread dequeue") { std::thread thr{[&queue]() { - std::this_thread::sleep_for(std::chrono::milliseconds{100}); queue.enqueue(DrawCommandsQueue::QueueItem{}); }}; + std::this_thread::sleep_for(std::chrono::milliseconds{100}); const auto item = queue.dequeue(); if (thr.joinable()) { thr.join(); } - REQUIRE(item.commands.empty()); - REQUIRE(item.refreshMode == ::gui::RefreshModes::GUI_REFRESH_FAST); + REQUIRE(item->commands.empty()); + REQUIRE(item->refreshMode == ::gui::RefreshModes::GUI_REFRESH_FAST); REQUIRE(queue.size() == 0U); } diff --git a/module-sys/Service/Worker.cpp b/module-sys/Service/Worker.cpp index 482c57b8b44d4ceb037e09b7aee7641bf0f2d6fd..18609a7a759ed0d7354eaeaa285f95f56fd64216 100644 --- a/module-sys/Service/Worker.cpp +++ b/module-sys/Service/Worker.cpp @@ -206,8 +206,6 @@ namespace sys { assert(getState() == State::Initiated); - runnerTask = xTaskGetCurrentTaskHandle(); - BaseType_t task_error = 0; task_error = xTaskCreate( Worker::taskAdapter, name.c_str(), stackDepth / sizeof(StackType_t), this, priority, &taskHandle); @@ -222,12 +220,8 @@ namespace sys bool Worker::stop() { - if (runnerTask) { - assert(xTaskGetCurrentTaskHandle() == runnerTask); - assert(getState() == State::Running); - return sendControlMessage(ControlMessage::Stop); - } - return true; + assert(getState() == State::Running); + return sendControlMessage(ControlMessage::Stop); } bool Worker::sendControlMessage(ControlMessage message) @@ -243,7 +237,6 @@ namespace sys bool Worker::send(uint32_t cmd, uint32_t *data) { - assert(xTaskGetCurrentTaskHandle() == runnerTask); assert(getState() == State::Running); WorkerCommand workerCommand{cmd, data}; @@ -275,15 +268,11 @@ namespace sys bool Worker::join(TickType_t timeout) { - if (runnerTask) { - assert(xTaskGetCurrentTaskHandle() == runnerTask); - assert(getState() == State::Running || getState() == State::Stopped); - - if (xSemaphoreTake(joinSemaphore, timeout) != pdTRUE) { - return false; - } - while (eTaskGetState(taskHandle) != eDeleted) {} + assert(getState() == State::Running || getState() == State::Stopped); + if (xSemaphoreTake(joinSemaphore, timeout) != pdTRUE) { + return false; } + while (eTaskGetState(taskHandle) != eDeleted) {} return true; } diff --git a/module-sys/Service/include/Service/Worker.hpp b/module-sys/Service/include/Service/Worker.hpp index 2d71a88a3022f4a046fc850bf594351288b2ee27..edcf85524b90a13335c8a82d07d70447126e41c4 100644 --- a/module-sys/Service/include/Service/Worker.hpp +++ b/module-sys/Service/include/Service/Worker.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2021, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once @@ -108,7 +108,6 @@ namespace sys static constexpr auto controlQueueNamePrefix = "wctrl"; xSemaphoreHandle joinSemaphore = nullptr; - xTaskHandle runnerTask = nullptr; xSemaphoreHandle stateMutex = nullptr; xTaskHandle taskHandle = nullptr; diff --git a/module-utils/log/Logger.cpp b/module-utils/log/Logger.cpp index 3fb4c64227c9bd417c73fe1dc3d88b24522cc691..4fa3c071a7a64a2617ebdbb0cd6da7f18a4e0c01 100644 --- a/module-utils/log/Logger.cpp +++ b/module-utils/log/Logger.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #include "Logger.hpp" @@ -166,19 +166,24 @@ namespace Log /// @return: < 0 - error occured during log flush /// @return: 0 - log flush did not happen /// @return: 1 - log flush successflul - auto Logger::dumpToFile(std::filesystem::path logPath) -> int + auto Logger::dumpToFile(std::filesystem::path logPath, bool isLoggerRunning) -> int { std::error_code errorCode; auto firstDump = !std::filesystem::exists(logPath, errorCode); if (errorCode) { - LOG_ERROR("Failed to check if file %s exists, error: %d", logPath.c_str(), errorCode.value()); + if (isLoggerRunning) { + LOG_ERROR("Failed to check if file %s exists, error: %d", logPath.c_str(), errorCode.value()); + } return -EIO; } if (const bool maxSizeExceeded = !firstDump && std::filesystem::file_size(logPath) > maxFileSize; maxSizeExceeded) { - LOG_DEBUG("Max log file size exceeded. Rotating log files..."); + if (isLoggerRunning) { + LOG_DEBUG("Max log file size exceeded. Rotating log files..."); + } + { LockGuard lock(logFileMutex); rotator.rotateFile(logPath); @@ -209,7 +214,9 @@ namespace Log } } - LOG_DEBUG("Flush ended with status: %d", status); + if (isLoggerRunning) { + LOG_DEBUG("Flush ended with status: %d", status); + } return status; } @@ -225,10 +232,10 @@ namespace Log auto Logger::flushLogs() -> int { LOG_INFO("Shutdown dump logs"); - worker->kill(); + worker->close(); writeLogsTimer.stop(); buffer.nextBuffer(); - return dumpToFile(purefs::dir::getLogsPath() / LOG_FILE_NAME); + return dumpToFile(purefs::dir::getLogsPath() / LOG_FILE_NAME, false); } void Logger::checkBufferState() diff --git a/module-utils/log/Logger.hpp b/module-utils/log/Logger.hpp index 1940c10358c69e30e3e59acf7420f3323969e41c..cd6cd811ee01f1a68a8a37d53cfd1180cc9bc4c7 100644 --- a/module-utils/log/Logger.hpp +++ b/module-utils/log/Logger.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once @@ -46,7 +46,7 @@ namespace Log auto log(logger_level level, const char *file, int line, const char *function, const char *fmt, va_list args) -> int; auto logAssert(const char *fmt, va_list args) -> int; - auto dumpToFile(std::filesystem::path logPath) -> int; + auto dumpToFile(std::filesystem::path logPath, bool isLoggerRunning = true) -> int; auto diagnosticDump() -> int; auto flushLogs() -> int;