From e2863061e01ef2ced699c5df0d47217bb8037df1 Mon Sep 17 00:00:00 2001 From: Jakub Pyszczak Date: Mon, 14 Jun 2021 13:59:16 +0200 Subject: [PATCH] [EGD-6932] Small cellular refactor Refactor regarding https://github.com/mudita/MuditaOS/pull/2226. --- module-cellular/modem/ATCommon.hpp | 9 +-- module-cellular/modem/mux/CellularMux.cpp | 63 ++++++------------- module-cellular/modem/mux/CellularMux.h | 9 ++- module-cellular/modem/mux/DLCChannel.cpp | 2 +- module-cellular/modem/mux/DLCChannel.h | 2 +- .../agents/settings/SettingsAgent.cpp | 4 +- module-sys/SystemManager/SystemManager.cpp | 2 +- 7 files changed, 32 insertions(+), 59 deletions(-) diff --git a/module-cellular/modem/ATCommon.hpp b/module-cellular/modem/ATCommon.hpp index 612098238..8df57eb79 100644 --- a/module-cellular/modem/ATCommon.hpp +++ b/module-cellular/modem/ATCommon.hpp @@ -3,6 +3,7 @@ #pragma once +#include #include #include #include @@ -26,25 +27,21 @@ namespace at struct AwaitingResponseFlag { private: - cpp_freertos::MutexStandard mutex; - bool isWaiting = false; + std::atomic_bool isWaiting = false; public: void set() { - cpp_freertos::LockGuard lock{mutex}; isWaiting = true; } void clear() { - cpp_freertos::LockGuard lock{mutex}; isWaiting = false; } - bool state() + bool state() const { - cpp_freertos::LockGuard lock{mutex}; return isWaiting; } }; diff --git a/module-cellular/modem/mux/CellularMux.cpp b/module-cellular/modem/mux/CellularMux.cpp index b8597b2b4..ea5a3d687 100644 --- a/module-cellular/modem/mux/CellularMux.cpp +++ b/module-cellular/modem/mux/CellularMux.cpp @@ -54,31 +54,11 @@ std::map ATPortSpeeds_text = {{PortSpeed_e::PS9600, 9 static constexpr std::uint16_t threadSizeWords = 2048; CellularMux::CellularMux(PortSpeed_e portSpeed, sys::Service *parent) -{ - init(portSpeed, parent); -} - -CellularMux::~CellularMux() -{ - for (auto it : channels) { - delete it; - } - channels.clear(); - - if (taskHandle != nullptr) { - vTaskDelete(taskHandle); - } - mode = Mode::AT; - - delete parser; -} - -void CellularMux::init(PortSpeed_e portSpeed, sys::Service *parent) { setStartParams(portSpeed); cellular = bsp::Cellular::create(SERIAL_PORT, portSpeed).value_or(nullptr); - parser = new ATParser(cellular.get()); + parser = std::make_unique(cellular.get()); parentService = parent; if (auto flushed = flushReceiveData(); flushed > 0) { @@ -95,10 +75,17 @@ void CellularMux::init(PortSpeed_e portSpeed, sys::Service *parent) xTaskCreate(workerTaskFunction, workerName, threadSizeWords, this, taskPriority, &taskHandle); if (taskError != pdPASS) { LOG_ERROR("Failed to start %s task", workerName); - return; } } +CellularMux::~CellularMux() +{ + if (taskHandle != nullptr) { + vTaskDelete(taskHandle); + } + mode = Mode::AT; +} + void CellularMux::setStartParams(PortSpeed_e portSpeed) { startParams.PortSpeed = portSpeed; @@ -182,22 +169,22 @@ CellularMux::ConfState CellularMux::baudDetectOnce() case BaudTestStep::baud460800_NoCmux: cellular->setSpeed(ATPortSpeeds_text[PortSpeed_e::PS460800]); lastStep = step; - result = baudDetectTestAT(parser, step, BaudTestStep::baud460800_Cmux); + result = baudDetectTestAT(parser.get(), step, BaudTestStep::baud460800_Cmux); break; case BaudTestStep::baud460800_Cmux: closeCMux(cellular); lastStep = step; - result = baudDetectTestAT(parser, step, BaudTestStep::baud115200_NoCmux); + result = baudDetectTestAT(parser.get(), step, BaudTestStep::baud115200_NoCmux); break; case BaudTestStep::baud115200_NoCmux: cellular->setSpeed(ATPortSpeeds_text[PortSpeed_e::PS115200]); lastStep = step; - result = baudDetectTestAT(parser, step, BaudTestStep::baud115200_Cmux); + result = baudDetectTestAT(parser.get(), step, BaudTestStep::baud115200_Cmux); break; case BaudTestStep::baud115200_Cmux: closeCMux(cellular); lastStep = step; - result = baudDetectTestAT(parser, step, BaudTestStep::baud_NotFound); + result = baudDetectTestAT(parser.get(), step, BaudTestStep::baud_NotFound); break; case BaudTestStep::baud_NotFound: cellular->setSpeed(ATPortSpeeds_text[PortSpeed_e::PS115200]); // set port speed to default 115200 @@ -462,7 +449,7 @@ void CellularMux::sendFrameToChannel(bsp::cellular::CellularResult &resultStruct { auto frame = CellularMuxFrame{resultStruct.getData()}; - for (auto chan : getChannels()) { + for (const auto &chan : channels) { if (frame.getFrameDLCI() == chan->getDLCI()) { if (frame.getData().empty()) { // Control frame contains no data @@ -773,27 +760,20 @@ CellularMux::ConfState CellularMux::setupEchoCanceller(EchoCancellerStrength str DLCChannel *CellularMux::get(Channel selectedChannel) { - for (auto channel : channels) { + for (const auto &channel : channels) { if (channel != nullptr && static_cast(channel->getDLCI()) == selectedChannel) { - return channel; + return channel.get(); } } return nullptr; } -std::vector &CellularMux::getChannels() -{ - return channels; -} - bool CellularMux::openChannel(Channel channelIndex) { - auto channel = new DLCChannel(static_cast(channelIndex), name(channelIndex), cellular.get()); - channels.push_back(channel); - - if (!channel->init()) { + channels.push_back( + std::make_unique(static_cast(channelIndex), name(channelIndex), cellular.get())); + if (!channels.back()->init()) { channels.pop_back(); - delete channel; return false; } @@ -802,9 +782,6 @@ bool CellularMux::openChannel(Channel channelIndex) void CellularMux::closeChannels() { - for (auto &it : channels) { - delete it; - } channels.clear(); mode = Mode::AT; } @@ -853,7 +830,7 @@ bsp::Cellular *CellularMux::getCellular() return cellular.get(); } -ATParser *CellularMux::getParser() +const std::unique_ptr &CellularMux::getParser() const { return parser; } diff --git a/module-cellular/modem/mux/CellularMux.h b/module-cellular/modem/mux/CellularMux.h index 92eb8e668..6efd9cdab 100644 --- a/module-cellular/modem/mux/CellularMux.h +++ b/module-cellular/modem/mux/CellularMux.h @@ -188,6 +188,7 @@ repeated until a response is obtained or action is taken by a higher layer. */ +#include #include #include #include @@ -258,12 +259,12 @@ class CellularMux MuxParameters startParams; sys::Service *parentService{}; std::unique_ptr cellular; - ATParser *parser{}; + std::unique_ptr parser; const uint32_t taskPriority = 0; xTaskHandle taskHandle = nullptr; - std::vector channels; + std::vector> channels; DLCChannel::Callback_t controlCallback = nullptr; friend void workerTaskFunction(void *ptr); @@ -291,7 +292,6 @@ class CellularMux ~CellularMux(); - void init(PortSpeed_e portSpeed, sys::Service *parent); void setStartParams(PortSpeed_e portSpeed); void setMode(Mode mode); @@ -300,7 +300,6 @@ class CellularMux /// @return pointer to channel or nullptr if such channel doesn't exist (nullptr return should never happen how - /// because all channels are opened once on start) DLCChannel *get(Channel selectedChannel); - std::vector &getChannels(); bool openChannel(Channel channelIndex); void closeChannels(); @@ -312,7 +311,7 @@ class CellularMux MuxParameters getStartParams() const noexcept; bsp::Cellular *getCellular(); - ATParser *getParser(); + [[nodiscard]] const std::unique_ptr &getParser() const; /// @brief It is searching the resposne for a string /// diff --git a/module-cellular/modem/mux/DLCChannel.cpp b/module-cellular/modem/mux/DLCChannel.cpp index 5721e3f2a..6c2e48433 100644 --- a/module-cellular/modem/mux/DLCChannel.cpp +++ b/module-cellular/modem/mux/DLCChannel.cpp @@ -154,7 +154,7 @@ std::vector DLCChannel::sendCommandPrompt(const char *cmd, return tokens; } -at::Result DLCChannel::parseInputData(bsp::cellular::CellularResult *cellularResult) +at::Result DLCChannel::parseInputData(bsp::cellular::CellularResult *cellularResult) const { at::Result result; diff --git a/module-cellular/modem/mux/DLCChannel.h b/module-cellular/modem/mux/DLCChannel.h index 24d52d91e..47b2d31b0 100644 --- a/module-cellular/modem/mux/DLCChannel.h +++ b/module-cellular/modem/mux/DLCChannel.h @@ -46,7 +46,7 @@ class DLCChannel : public at::Channel size_t rxCount, std::chrono::milliseconds timeout = std::chrono::milliseconds{300}); - at::Result parseInputData(bsp::cellular::CellularResult *cellularResult); + at::Result parseInputData(bsp::cellular::CellularResult *cellularResult) const; bool evaluateEstablishResponse(bsp::cellular::CellularResult &response) const; diff --git a/module-services/service-db/agents/settings/SettingsAgent.cpp b/module-services/service-db/agents/settings/SettingsAgent.cpp index eb7d2561f..cf0c9d2a9 100644 --- a/module-services/service-db/agents/settings/SettingsAgent.cpp +++ b/module-services/service-db/agents/settings/SettingsAgent.cpp @@ -169,12 +169,12 @@ auto SettingsAgent::handleRegisterOnVariableChange(sys::Message *req) -> sys::Me auto SettingsAgent::handleUnregisterOnVariableChange(sys::Message *req) -> sys::MessagePointer { - if (auto msg = dynamic_cast(req)) { + if (auto msg = dynamic_cast(req); msg != nullptr) { auto path = msg->getPath(); if (dbUnregisterValueChange(path)) { if (auto it = variableChangeRecipients.find(path.to_string()); it != variableChangeRecipients.end()) { - it->second.erase(path); LOG_DEBUG("[SettingsAgent::handleUnregisterOnVariableChange] %s", path.to_string().c_str()); + it->second.erase(path); } } } diff --git a/module-sys/SystemManager/SystemManager.cpp b/module-sys/SystemManager/SystemManager.cpp index 169ec1a15..918fdaf08 100644 --- a/module-sys/SystemManager/SystemManager.cpp +++ b/module-sys/SystemManager/SystemManager.cpp @@ -159,7 +159,7 @@ namespace sys LOG_FATAL("State changed after reset/shutdown was requested to: %s! this is terrible failure!", c_str(state)); exit(1); - }; + } } void SystemManager::initialize()