From 68be7290fdca4a11b18eb1bb16e7ee3ce3781168 Mon Sep 17 00:00:00 2001 From: aw Date: Thu, 26 Sep 2024 21:21:41 +0200 Subject: [PATCH 1/2] refactor(EvseManager): more generic error checking - added utility function `util::find_first_match`, which behaves similar to `std::visit`: - it is given a matcher class as the first argument and a variadic amount of additional arguments, that possibly can have different types - the matcher class needs to overload its call operator for each of these types and implement a corresponding matching function - `find_first_match` will then execute the corresponding matching function for each argument in order and return the first value, which evaluates to true - this utility function is used to write the check for active fatal errors within the EvseManager in a more generic fashion Signed-off-by: aw --- modules/EvseManager/ErrorHandling.cpp | 95 +++++++++++------------ modules/EvseManager/lib/error_defs.hpp | 27 +++++++ modules/EvseManager/lib/util.hpp | 14 ++++ modules/EvseManager/tests/CMakeLists.txt | 12 +++ modules/EvseManager/tests/util.cpp | 96 ++++++++++++++++++++++++ 5 files changed, 191 insertions(+), 53 deletions(-) create mode 100644 modules/EvseManager/lib/error_defs.hpp create mode 100644 modules/EvseManager/lib/util.hpp create mode 100644 modules/EvseManager/tests/util.cpp diff --git a/modules/EvseManager/ErrorHandling.cpp b/modules/EvseManager/ErrorHandling.cpp index 2f6ac049e..2e11ebd5d 100644 --- a/modules/EvseManager/ErrorHandling.cpp +++ b/modules/EvseManager/ErrorHandling.cpp @@ -3,19 +3,10 @@ #include "ErrorHandling.hpp" -namespace module { +#include "lib/error_defs.hpp" +#include "lib/util.hpp" -using ErrorList = std::list; -static const struct IgnoreErrors { - // p_evse. We need to ignore Inoperative here as this is the result of this check. - ErrorList evse{"evse_manager/Inoperative"}; - ErrorList bsp{"evse_board_support/MREC3HighTemperature", "evse_board_support/MREC18CableOverTempDerate", - "evse_board_support/VendorWarning"}; - ErrorList connector_lock{"connector_lock/VendorWarning"}; - ErrorList ac_rcd{"ac_rcd/VendorWarning"}; - ErrorList imd{"isolation_monitor/VendorWarning"}; - ErrorList powersupply{"power_supply_DC/VendorWarning"}; -} ignore_errors; +namespace module { ErrorHandling::ErrorHandling(const std::unique_ptr& _r_bsp, const std::vector>& _r_hlc, @@ -109,57 +100,55 @@ void ErrorHandling::process_error() { } } -// Check all errors from p_evse and all requirements to see if they block charging -std::optional ErrorHandling::errors_prevent_charging() { - - auto is_fatal = [](auto errors, auto ignore_list) -> std::optional { - for (const auto e : errors) { - if (std::none_of(ignore_list.begin(), ignore_list.end(), [e](const auto& ign) { return e->type == ign; })) { - return e->type; - } +template struct ErrorView { + const T& source; + const ErrorList& to_ignore; +}; +template ErrorView(T&, const ErrorList&) -> ErrorView; + +std::optional find_first_error(const std::list& active_errors, + const ErrorList& to_ignore) { + for (const auto& error : active_errors) { + if (std::none_of(std::begin(to_ignore), std::end(to_ignore), + [&error](const auto& error_to_ignore) { return error->type == error_to_ignore; })) { + return error->type; } - return std::nullopt; - }; - - auto fatal = is_fatal(p_evse->error_state_monitor->get_active_errors(), ignore_errors.evse); - if (fatal) { - return fatal; } - fatal = is_fatal(r_bsp->error_state_monitor->get_active_errors(), ignore_errors.bsp); - if (fatal) { - return fatal; - } - - if (r_connector_lock.size() > 0) { - fatal = is_fatal(r_connector_lock[0]->error_state_monitor->get_active_errors(), ignore_errors.connector_lock); - if (fatal) { - return fatal; - } - } + return std::nullopt; +} - if (r_ac_rcd.size() > 0) { - fatal = is_fatal(r_ac_rcd[0]->error_state_monitor->get_active_errors(), ignore_errors.ac_rcd); - if (fatal) { - return fatal; - } - } +struct Matcher { + using return_type = std::optional; - if (r_imd.size() > 0) { - fatal = is_fatal(r_imd[0]->error_state_monitor->get_active_errors(), ignore_errors.imd); - if (fatal) { - return fatal; - } + template return_type operator()(const T& error_view) { + return find_first_error(error_view.source.error_state_monitor->get_active_errors(), error_view.to_ignore); } - if (r_powersupply.size() > 0) { - fatal = is_fatal(r_powersupply[0]->error_state_monitor->get_active_errors(), ignore_errors.powersupply); - if (fatal) { - return fatal; + // specialization for requirement lists + template return_type operator()(const ErrorView>& error_view) { + for (const auto& elem : error_view.source) { + auto match = find_first_error(elem->error_state_monitor->get_active_errors(), error_view.to_ignore); + if (match) { + return match; + } } + return std::nullopt; } +}; - return std::nullopt; +// Check all errors from p_evse and all requirements to see if they block charging +std::optional ErrorHandling::errors_prevent_charging() { + /* clang-format off */ + return ev::util::find_first_match(Matcher(), + ErrorView{*p_evse, errors_to_ignore::evse}, + ErrorView{*r_bsp, errors_to_ignore::bsp}, + ErrorView{r_connector_lock, errors_to_ignore::connector_lock}, + ErrorView{r_ac_rcd, errors_to_ignore::ac_rcd}, + ErrorView{r_imd, errors_to_ignore::imd}, + ErrorView{r_powersupply, errors_to_ignore::powersupply} + ); + /* clang-format on */ } void ErrorHandling::raise_inoperative_error(const std::string& caused_by) { diff --git a/modules/EvseManager/lib/error_defs.hpp b/modules/EvseManager/lib/error_defs.hpp new file mode 100644 index 000000000..e82f0f6d7 --- /dev/null +++ b/modules/EvseManager/lib/error_defs.hpp @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Pionix GmbH and Contributors to EVerest +#pragma once + +#include + +// FIXME (aw): everest-framework imports are not properly namespaced +#include + +namespace module { + +using ErrorList = std::initializer_list; + +namespace errors_to_ignore { +// FIXME (aw): these strings should be strongly typed (propably via autogeneration) instead of repeatedly hardcoded +// p_evse. We need to ignore Inoperative here as this is the result of this check. +inline const ErrorList evse{"evse_manager/Inoperative"}; +inline const ErrorList bsp{"evse_board_support/MREC3HighTemperature", "evse_board_support/MREC18CableOverTempDerate", + "evse_board_support/VendorWarning"}; +inline const ErrorList connector_lock{"connector_lock/VendorWarning"}; +inline const ErrorList ac_rcd{"ac_rcd/VendorWarning"}; +inline const ErrorList imd{"isolation_monitor/VendorWarning"}; +inline const ErrorList powersupply{"power_supply_DC/VendorWarning"}; + +} // namespace errors_to_ignore + +} // namespace module \ No newline at end of file diff --git a/modules/EvseManager/lib/util.hpp b/modules/EvseManager/lib/util.hpp new file mode 100644 index 000000000..db3143619 --- /dev/null +++ b/modules/EvseManager/lib/util.hpp @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Pionix GmbH and Contributors to EVerest +#pragma once + +namespace ev::util { + +template auto find_first_match(Matcher&& matcher, Args&&... sets) { + typename Matcher::return_type match; + // the following line makes use of short-circuiting in boolean expression + (((match = matcher(std::forward(sets))) || ...)); + return match; +} + +} // namespace ev::util \ No newline at end of file diff --git a/modules/EvseManager/tests/CMakeLists.txt b/modules/EvseManager/tests/CMakeLists.txt index 2bccddf3d..be255c264 100644 --- a/modules/EvseManager/tests/CMakeLists.txt +++ b/modules/EvseManager/tests/CMakeLists.txt @@ -31,3 +31,15 @@ target_link_libraries(${TEST_TARGET_NAME} PRIVATE ) add_test(${TEST_TARGET_NAME} ${TEST_TARGET_NAME}) + +set(UTIL_TEST_TARGET ${MODULE_NAME}_util) +add_executable(${UTIL_TEST_TARGET} util.cpp) + +target_compile_definitions(${UTIL_TEST_TARGET} PRIVATE + BUILD_TESTING_MODULE_EVSE_MANAGER +) + +target_link_libraries(${UTIL_TEST_TARGET} + PRIVATE + Catch2::Catch2WithMain +) diff --git a/modules/EvseManager/tests/util.cpp b/modules/EvseManager/tests/util.cpp new file mode 100644 index 000000000..bfb494cec --- /dev/null +++ b/modules/EvseManager/tests/util.cpp @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Pionix GmbH and Contributors to EVerest +#include + +#include +#include + +#include "../lib/util.hpp" + +using namespace ev::util; + +struct SenseOfLive { + int value; + static constexpr auto correct_value = 42; +}; + +struct LatchCheck { + bool valid; + bool got_checked; + static constexpr auto return_value = -42; +}; +struct Matcher { + using return_type = std::optional; + + return_type operator()(const SenseOfLive& item) { + if (item.value == SenseOfLive::correct_value) { + return item.value; + } + + return std::nullopt; + } + + return_type operator()(LatchCheck& item) { + item.got_checked = true; + if (item.valid) { + return LatchCheck::return_value; + } + + return std::nullopt; + } +}; + +SCENARIO("feeding find_first_match with various arguments", "[util::find_first_match]") { + WHEN("no matchables are supplied") { + const auto result = find_first_match(Matcher()); + + THEN("the result should be the default constructed return value") { + REQUIRE(result == Matcher::return_type()); + } + } + WHEN("all matchables are false") { + const auto result = find_first_match(Matcher(), SenseOfLive{23}, SenseOfLive{12}); + + THEN("there should be no match") { + REQUIRE(not result); + } + } + + WHEN("a matchable is correct") { + const auto result = + find_first_match(Matcher(), SenseOfLive{13}, SenseOfLive{SenseOfLive::correct_value}, SenseOfLive{12}); + + THEN("there should be a match") { + REQUIRE(result); + } + + AND_THEN("it should have the correct value") { + REQUIRE(*result == SenseOfLive::correct_value); + } + } + + WHEN("the first matchable is correct") { + LatchCheck latch_check{true}; + + const auto result = find_first_match(Matcher(), SenseOfLive{SenseOfLive::correct_value}, latch_check); + + THEN("the second matchable shouldn't have been tested") { + REQUIRE(latch_check.got_checked == false); + } + } + + WHEN("the first matchable is not correct") { + LatchCheck latch_check{true}; + + const auto result = find_first_match(Matcher(), SenseOfLive{0}, latch_check); + + THEN("the second matchabe should be checked") { + REQUIRE(latch_check.got_checked == true); + } + + AND_THEN("it should have the correct value") { + REQUIRE(result); + REQUIRE(*result == LatchCheck::return_value); + } + } +} From 6a49e4fe0950a18c5111a6f02a4a9341d818550d Mon Sep 17 00:00:00 2001 From: aw Date: Wed, 6 Nov 2024 10:52:44 +0100 Subject: [PATCH 2/2] fix bazel build Signed-off-by: aw --- modules/EvseManager/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/EvseManager/BUILD.bazel b/modules/EvseManager/BUILD.bazel index 62368b608..f0af5b5c6 100644 --- a/modules/EvseManager/BUILD.bazel +++ b/modules/EvseManager/BUILD.bazel @@ -16,6 +16,7 @@ cc_everest_module( impls = IMPLS, srcs = glob( [ + "lib/*.hpp", "*.cpp", "*.hpp", ],