From c26999ca98e9c8cc838f19bfa612bc9bb8bbf09e Mon Sep 17 00:00:00 2001 From: James Chapman <147724513+james-ctc@users.noreply.github.com> Date: Tue, 1 Oct 2024 12:34:08 +0100 Subject: [PATCH] fix: API module; delay sending commands to EvseManager until it is ready (#856) * fix: API module; delay sending commands to EvseManager until it is ready Signed-off-by: James Chapman * fix: allow for 128 EvseManagers Signed-off-by: James Chapman * fix: approach updated to allow multiple `ready` events from the same EVSE manager Signed-off-by: James Chapman * fix: update to latest main and fix compile/merge issue Signed-off-by: James Chapman --------- Signed-off-by: James Chapman --- modules/API/API.cpp | 32 ++++-- modules/API/API.hpp | 4 + modules/API/CMakeLists.txt | 4 + modules/API/StartupMonitor.cpp | 76 +++++++++++++ modules/API/StartupMonitor.hpp | 68 ++++++++++++ modules/API/tests/CMakeLists.txt | 19 ++++ modules/API/tests/StartupMonitor_test.cpp | 124 ++++++++++++++++++++++ tests/include/everest/logging.hpp | 44 ++++++++ 8 files changed, 363 insertions(+), 8 deletions(-) create mode 100644 modules/API/StartupMonitor.cpp create mode 100644 modules/API/StartupMonitor.hpp create mode 100644 modules/API/tests/CMakeLists.txt create mode 100644 modules/API/tests/StartupMonitor_test.cpp create mode 100644 tests/include/everest/logging.hpp diff --git a/modules/API/API.cpp b/modules/API/API.cpp index 361b179ba..68c4900df 100644 --- a/modules/API/API.cpp +++ b/modules/API/API.cpp @@ -269,12 +269,20 @@ void API::init() { std::vector connectors; std::string var_connectors = this->api_base + "connectors"; + evse_manager_check.set_total(r_evse_manager.size()); + for (auto& evse : this->r_evse_manager) { auto& session_info = this->info.emplace_back(std::make_unique()); auto& hw_caps = this->hw_capabilities_str.emplace_back(""); std::string evse_base = this->api_base + evse->module_id; connectors.push_back(evse->module_id); + evse->subscribe_ready([this, &evse](bool ready) { + if (ready) { + this->evse_manager_check.notify_ready(evse->module_id); + } + }); + // API variables std::string var_base = evse_base + "/var/"; @@ -395,7 +403,7 @@ void API::init() { std::string cmd_base = evse_base + "/cmd/"; std::string cmd_enable_disable = cmd_base + "enable_disable"; - this->mqtt.subscribe(cmd_enable_disable, [&evse](const std::string& data) { + this->mqtt.subscribe(cmd_enable_disable, [this, &evse](const std::string& data) { auto connector_id = 0; types::evse_manager::EnableDisableSource enable_source{types::evse_manager::Enable_source::LocalAPI, types::evse_manager::Enable_state::Enable, 100}; @@ -422,11 +430,12 @@ void API::init() { } else { EVLOG_error << "enable: No argument specified, ignoring command"; } + this->evse_manager_check.wait_ready(); evse->call_enable_disable(connector_id, enable_source); }); std::string cmd_disable = cmd_base + "disable"; - this->mqtt.subscribe(cmd_disable, [&evse](const std::string& data) { + this->mqtt.subscribe(cmd_disable, [this, &evse](const std::string& data) { auto connector_id = 0; types::evse_manager::EnableDisableSource enable_source{types::evse_manager::Enable_source::LocalAPI, types::evse_manager::Enable_state::Disable, 100}; @@ -441,11 +450,12 @@ void API::init() { } else { EVLOG_error << "disable: No argument specified, ignoring command"; } + this->evse_manager_check.wait_ready(); evse->call_enable_disable(connector_id, enable_source); }); std::string cmd_enable = cmd_base + "enable"; - this->mqtt.subscribe(cmd_enable, [&evse](const std::string& data) { + this->mqtt.subscribe(cmd_enable, [this, &evse](const std::string& data) { auto connector_id = 0; types::evse_manager::EnableDisableSource enable_source{types::evse_manager::Enable_source::LocalAPI, types::evse_manager::Enable_state::Enable, 100}; @@ -460,23 +470,27 @@ void API::init() { } else { EVLOG_error << "disable: No argument specified, ignoring command"; } + this->evse_manager_check.wait_ready(); evse->call_enable_disable(connector_id, enable_source); }); std::string cmd_pause_charging = cmd_base + "pause_charging"; - this->mqtt.subscribe(cmd_pause_charging, [&evse](const std::string& data) { + this->mqtt.subscribe(cmd_pause_charging, [this, &evse](const std::string& data) { + this->evse_manager_check.wait_ready(); evse->call_pause_charging(); // }); std::string cmd_resume_charging = cmd_base + "resume_charging"; - this->mqtt.subscribe(cmd_resume_charging, [&evse](const std::string& data) { + this->mqtt.subscribe(cmd_resume_charging, [this, &evse](const std::string& data) { + this->evse_manager_check.wait_ready(); evse->call_resume_charging(); // }); std::string cmd_set_limit = cmd_base + "set_limit_amps"; - this->mqtt.subscribe(cmd_set_limit, [&evse](const std::string& data) { + this->mqtt.subscribe(cmd_set_limit, [this, &evse](const std::string& data) { try { const auto external_limits = get_external_limits(data, false); + this->evse_manager_check.wait_ready(); evse->call_set_external_limits(external_limits); } catch (const std::invalid_argument& e) { EVLOG_warning << "Invalid limit: No conversion of given input could be performed."; @@ -486,9 +500,10 @@ void API::init() { }); std::string cmd_set_limit_watts = cmd_base + "set_limit_watts"; - this->mqtt.subscribe(cmd_set_limit_watts, [&evse](const std::string& data) { + this->mqtt.subscribe(cmd_set_limit_watts, [this, &evse](const std::string& data) { try { const auto external_limits = get_external_limits(data, true); + this->evse_manager_check.wait_ready(); evse->call_set_external_limits(external_limits); } catch (const std::invalid_argument& e) { EVLOG_warning << "Invalid limit: No conversion of given input could be performed."; @@ -497,7 +512,7 @@ void API::init() { } }); std::string cmd_force_unlock = cmd_base + "force_unlock"; - this->mqtt.subscribe(cmd_force_unlock, [&evse](const std::string& data) { + this->mqtt.subscribe(cmd_force_unlock, [this, &evse](const std::string& data) { int connector_id = 1; if (!data.empty()) { try { @@ -512,6 +527,7 @@ void API::init() { // perform the same action types::evse_manager::StopTransactionRequest req; req.reason = types::evse_manager::StopTransactionReason::UnlockCommand; + this->evse_manager_check.wait_ready(); evse->call_stop_transaction(req); evse->call_force_unlock(connector_id); }); diff --git a/modules/API/API.hpp b/modules/API/API.hpp index 5aa7b1329..7f83c9ec3 100644 --- a/modules/API/API.hpp +++ b/modules/API/API.hpp @@ -21,6 +21,7 @@ // ev@4bf81b14-a215-475c-a1d3-0a484ae48918:v1 // insert your custom include headers here +#include #include #include #include @@ -29,6 +30,7 @@ #include #include +#include "StartupMonitor.hpp" #include "limit_decimal_places.hpp" namespace module { @@ -192,6 +194,8 @@ class API : public Everest::ModuleBase { std::vector api_threads; bool running = true; + StartupMonitor evse_manager_check; + std::list> info; std::list hw_capabilities_str; std::string selected_protocol; diff --git a/modules/API/CMakeLists.txt b/modules/API/CMakeLists.txt index 7146700eb..6f0333fb6 100644 --- a/modules/API/CMakeLists.txt +++ b/modules/API/CMakeLists.txt @@ -16,6 +16,7 @@ target_link_libraries(${MODULE_NAME} target_sources(${MODULE_NAME} PRIVATE "limit_decimal_places.cpp" + "StartupMonitor.cpp" ) # ev@bcc62523-e22b-41d7-ba2f-825b493a3c97:v1 @@ -26,4 +27,7 @@ target_sources(${MODULE_NAME} # ev@c55432ab-152c-45a9-9d2e-7281d50c69c3:v1 # insert other things like install cmds etc here +if(EVEREST_CORE_BUILD_TESTING) + add_subdirectory(tests) +endif() # ev@c55432ab-152c-45a9-9d2e-7281d50c69c3:v1 diff --git a/modules/API/StartupMonitor.cpp b/modules/API/StartupMonitor.cpp new file mode 100644 index 000000000..7799f3b69 --- /dev/null +++ b/modules/API/StartupMonitor.cpp @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Pionix GmbH and Contributors to EVerest + +#include "StartupMonitor.hpp" +#include + +#include + +namespace module { + +bool StartupMonitor::check_ready() { + bool result{false}; + if (ready_set) { + result = ready_set->size() >= n_managers; + } + return result; +} + +bool StartupMonitor::set_total(std::uint8_t total) { + bool result{true}; + { + std::lock_guard lock(mutex); + if (!ready_set) { + n_managers = total; + if (total == 0) { + managers_ready = true; + } else { + managers_ready = false; + ready_set = std::make_unique(); + } + } else { + // already set + EVLOG_error << "Invalid attempt to set number of EVSE managers"; + result = false; + } + } + if (total == 0) { + cv.notify_all(); + } + return result; +} + +void StartupMonitor::wait_ready() { + std::unique_lock lock(mutex); + cv.wait(lock, [this] { return this->managers_ready; }); +} + +bool StartupMonitor::notify_ready(const std::string& evse_manager_id) { + bool result{true}; + bool notify{false}; + { + std::lock_guard lock(mutex); + if (ready_set) { + ready_set->insert(evse_manager_id); + notify = StartupMonitor::check_ready(); + if (notify) { + managers_ready = true; + n_managers = 0; + ready_set->clear(); // reclaim memory + } + } else { + result = false; + if (managers_ready) { + EVLOG_warning << "EVSE manager ready after complete"; + } else { + EVLOG_error << "EVSE manager ready before total number set"; + } + } + } + if (notify) { + cv.notify_all(); + } + return result; +} + +} // namespace module diff --git a/modules/API/StartupMonitor.hpp b/modules/API/StartupMonitor.hpp new file mode 100644 index 000000000..023552001 --- /dev/null +++ b/modules/API/StartupMonitor.hpp @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Pionix GmbH and Contributors to EVerest +#ifndef STARTUPMONITOR_HPP +#define STARTUPMONITOR_HPP + +#include +#include +#include +#include +#include +#include + +namespace module { + +/** + * \brief collect ready responses from all EVSE managers + * + * Provides a mechanism for API code to wait for all EVSE managers to be ready. + * Every EVSE manager is expected to set a `ready` variable to true. This class + * collects the IDs of EVSE managers to check that the expected number are + * ready before allowing API calls to proceed. + * + * \note an EVSE manager is not expected to set `ready` more than once, however + * this class manages this so that the `ready` is only counted once. + */ +class StartupMonitor { +private: + using ready_t = std::set; + + std::condition_variable cv; + std::mutex mutex; + +protected: + std::unique_ptr ready_set; //!< set of received ready responses + std::uint16_t n_managers{0}; //!< total number of EVSE managers + bool managers_ready{false}; //!< all EVSE managers are ready + + /** + * \brief check whether all ready responses have been received + * \returns true when the ready set contains at least n_managers responses + */ + bool check_ready(); + +public: + /** + * \brief set the total number of EVSE managers + * \param[in] total the number of EVSE managers + * \returns false if the total has already been set + */ + bool set_total(std::uint8_t total); + + /** + * \brief wait for all EVSE managers to be ready + */ + void wait_ready(); + + /** + * \brief notify that a specific EVSE manager is ready + * \param[in] evse_manager_id the ID of the EVSE manager + * \returns false if the total has not been set + * \note notify_ready() may be called multiple times with the same evse_manager_id + */ + bool notify_ready(const std::string& evse_manager_id); +}; + +} // namespace module + +#endif // STARTUPMONITOR_HPP diff --git a/modules/API/tests/CMakeLists.txt b/modules/API/tests/CMakeLists.txt new file mode 100644 index 000000000..b3519bf91 --- /dev/null +++ b/modules/API/tests/CMakeLists.txt @@ -0,0 +1,19 @@ +set(TEST_TARGET_NAME ${PROJECT_NAME}_API_tests) +add_executable(${TEST_TARGET_NAME}) + +add_dependencies(${TEST_TARGET_NAME} ${MODULE_NAME}) + +target_include_directories(${TEST_TARGET_NAME} PRIVATE + . .. ../../../tests/include +) + +target_sources(${TEST_TARGET_NAME} PRIVATE + StartupMonitor_test.cpp + ../StartupMonitor.cpp +) + +target_link_libraries(${TEST_TARGET_NAME} PRIVATE + GTest::gtest_main +) + +add_test(${TEST_TARGET_NAME} ${TEST_TARGET_NAME}) diff --git a/modules/API/tests/StartupMonitor_test.cpp b/modules/API/tests/StartupMonitor_test.cpp new file mode 100644 index 000000000..55ddec705 --- /dev/null +++ b/modules/API/tests/StartupMonitor_test.cpp @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Pionix GmbH and Contributors to EVerest +#include + +#include "StartupMonitor.hpp" + +#include + +namespace { +using namespace module; + +struct StartupMonitorTest : public StartupMonitor { + [[nodiscard]] constexpr bool startup_complete() const { + return managers_ready; + } + [[nodiscard]] constexpr std::uint8_t total() const { + return n_managers; + } + [[nodiscard]] inline std::uint8_t startup_count() const { + return (ready_set) ? ready_set->size() : 0; + } +}; + +TEST(StartupMonitor, init) { + StartupMonitorTest startup; + EXPECT_FALSE(startup.startup_complete()); + EXPECT_EQ(startup.startup_count(), 0); + EXPECT_EQ(startup.total(), 0); + + bool woken{false}; + std::thread thread([&startup, &woken]() { + startup.wait_ready(); + woken = true; + }); + + EXPECT_FALSE(woken); + + EXPECT_TRUE(startup.set_total(1)); + EXPECT_EQ(startup.total(), 1); + EXPECT_FALSE(woken); + EXPECT_TRUE(startup.notify_ready("manager1")); + // EXPECT_EQ(startup.startup_count(), 1); will be 0 because startup is complete + thread.join(); + EXPECT_TRUE(woken); + EXPECT_TRUE(startup.startup_complete()); + EXPECT_EQ(startup.total(), 0); +} + +TEST(StartupMonitor, zero) { + StartupMonitorTest startup; + EXPECT_FALSE(startup.startup_complete()); + EXPECT_EQ(startup.startup_count(), 0); + EXPECT_EQ(startup.total(), 0); + + bool woken{false}; + std::thread thread([&startup, &woken]() { + startup.wait_ready(); + woken = true; + }); + + EXPECT_FALSE(woken); + + EXPECT_TRUE(startup.set_total(0)); + EXPECT_EQ(startup.total(), 0); + EXPECT_EQ(startup.startup_count(), 0); + thread.join(); + EXPECT_TRUE(woken); + EXPECT_TRUE(startup.startup_complete()); + EXPECT_EQ(startup.total(), 0); +} + +TEST(StartupMonitor, invalidSequence) { + StartupMonitorTest startup; + EXPECT_FALSE(startup.startup_complete()); + EXPECT_FALSE(startup.notify_ready("manager1")); // total not set yet + EXPECT_TRUE(startup.set_total(1)); + EXPECT_EQ(startup.startup_count(), 0); + EXPECT_EQ(startup.total(), 1); + + bool woken{false}; + std::thread thread([&startup, &woken]() { + startup.wait_ready(); + woken = true; + }); + + EXPECT_FALSE(startup.set_total(2)); // total already set + EXPECT_EQ(startup.total(), 1); // didn't change + EXPECT_TRUE(startup.notify_ready("manager2")); + // EXPECT_EQ(startup.startup_count(), 1); will be 0 because startup is complete + thread.join(); + EXPECT_TRUE(woken); + EXPECT_TRUE(startup.startup_complete()); + EXPECT_EQ(startup.total(), 0); +} + +TEST(StartupMonitor, duplicateReady) { + StartupMonitorTest startup; + EXPECT_FALSE(startup.startup_complete()); + EXPECT_TRUE(startup.set_total(2)); + EXPECT_EQ(startup.startup_count(), 0); + EXPECT_EQ(startup.total(), 2); + + bool woken{false}; + std::thread thread([&startup, &woken]() { + startup.wait_ready(); + woken = true; + }); + + EXPECT_TRUE(startup.notify_ready("manager1")); + EXPECT_EQ(startup.startup_count(), 1); + EXPECT_TRUE(startup.notify_ready("manager1")); // duplicate + EXPECT_EQ(startup.startup_count(), 1); + EXPECT_FALSE(startup.startup_complete()); + EXPECT_TRUE(startup.notify_ready("manager2")); + // EXPECT_EQ(startup.startup_count(), 2); will be 0 because startup is complete + EXPECT_TRUE(startup.startup_complete()); + + thread.join(); + EXPECT_TRUE(woken); + EXPECT_TRUE(startup.startup_complete()); + EXPECT_EQ(startup.total(), 0); +} + +} // namespace diff --git a/tests/include/everest/logging.hpp b/tests/include/everest/logging.hpp new file mode 100644 index 000000000..c245fde54 --- /dev/null +++ b/tests/include/everest/logging.hpp @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Pionix GmbH and Contributors to EVerest + +#pragma once + +#include +#include + +namespace { +class UTestLogger { +private: + std::ostream& m_stream; + +public: + UTestLogger() = delete; + UTestLogger(const char* file, int line) : UTestLogger(std::cerr, "Error: ", file, line) { + } + UTestLogger(std::ostream& stream, const char* level, const char* file, int line) : m_stream(stream) { + const auto f = std::filesystem::path((file == nullptr) ? "(unknown)" : file); + m_stream << level << f.filename().string() << ':' << line << ' '; + } + ~UTestLogger() { + m_stream << std::endl; + } + template constexpr std::ostream& operator<<(const T& item) { + return m_stream << item; + } +}; + +#define EVLOG_critical UTestLogger(std::cerr, "Critical: ", __FILE__, __LINE__) +#define EVLOG_error UTestLogger(std::cerr, "Error: ", __FILE__, __LINE__) +#define EVLOG_warning UTestLogger(std::cout, "Warning: ", __FILE__, __LINE__) +#define EVLOG_info UTestLogger(std::cout, "Info: ", __FILE__, __LINE__) +#define EVLOG_debug UTestLogger(std::cout, "Debug: ", __FILE__, __LINE__) +#define EVLOG_AND_THROW(ex) \ + do { \ + try { \ + throw ex; \ + } catch (std::exception & e) { \ + EVLOG_error << e.what(); \ + throw; \ + } \ + } while (0) +} // namespace