Adding Ocpp Configuration Module #799
Conversation
| #include "mapping_reader.hpp" | ||
| #include "util.hpp" | ||
| #include <iostream> |
There was a problem hiding this comment.
Done but I think this should be done automatically by some tool instead of having to do it manually. Isn't this set up somehow already?
MarzellT
force-pushed
the
tm-adding-ocpp-configuration-module
branch
2 times, most recently
from
4fa1426 to
475e176
Compare
MarzellT
requested review from
SirVer,
golovasteek,
dorezyuk,
hikinggrass,
corneliusclaussen and
Pietfried
as code owners
MarzellT
force-pushed
the
tm-adding-ocpp-configuration-module
branch
2 times, most recently
from
72147ca to
2b86368
Compare
|
|
||
| private: | ||
| const std::optional<EverestConfigMapping> | ||
| find_mapping_by_component_variable_or_log_error(const types::ocpp::ComponentVariable& component_variable) const; |
There was a problem hiding this comment.
It should be considered normal to receive event data for variables that are not part of the mapping, so I wouldnt throw a runtime_error exception here but simply do nothing in case nothing can be found in the mapping. I think its enough to only have this function:
const std::optional<EverestConfigMapping> find_mapping_by_component_variable(const types::ocpp::ComponentVariable& component_variable) const;
There was a problem hiding this comment.
I decided to keep both functions but change the high level function to not log any error.
My reasoning was that the high level function checks if the low level function returned a value and if not returns a nullopt and the other function makes the actual lookup.
This way both functions have just one responsibility and I can still use the raii approach in the caller function of the high level function
| namespace module { | ||
|
|
||
| void OCPPConfiguration::init() { | ||
| invoke_init(*p_main); |
There was a problem hiding this comment.
You should verify the configuration parameters in the init:
- Check if mapping file exists and is valid and warn if it doesn't
- Check if user config exists and warn if it doesn't
In case the configuration could not be verified this module is not operational, but it should not crash, since it doesn't provide any interface and shold not cause a crash of everest ever.
There was a problem hiding this comment.
The first case is handled implicitly now but I think the second case shouldn't generate a warning and just silently create the file. Do you disagree?
| namespace module { | ||
| OcppToEverestConfigMapping MappingReader::read_mapping(const std::filesystem::path& file_path) { | ||
| const auto hacked_file_path = | ||
| std::filesystem::path{"modules"} / "OCPPConfiguration" / file_path; // TODO: this is very hacky |
There was a problem hiding this comment.
yes ;) you could pass the mapping file path in the constructor of the class
There was a problem hiding this comment.
I completely changed this and I think the solution is good, so this comment would be irrelevant then
| throw std::runtime_error("EVSE node must have an id"); | ||
| } | ||
| const auto id_val = node["id"].val(); | ||
| auto id = std::stoi(std::string{id_val.str, id_val.len}); |
There was a problem hiding this comment.
This should catch std::invalid_argument
|
|
||
| const auto connector_id_optional_val = node.has_child("id") ? std::optional{node["id"].val()} : std::nullopt; | ||
| auto connector_id = connector_id_optional_val.has_value() | ||
| ? std::optional{std::stoi(std::string{connector_id_optional_val.value().str, |
There was a problem hiding this comment.
This should catch std::invalid_argument
MarzellT
force-pushed
the
tm-adding-ocpp-configuration-module
branch
2 times, most recently
from
a27fc7f to
a7cb4be
Compare
MarzellT
force-pushed
the
tm-adding-ocpp-configuration-module
branch
2 times, most recently
from
85c6c08 to
cae1e6b
Compare
MarzellT
force-pushed
the
tm-adding-ocpp-configuration-module
branch
from
f6022fe to
83f3de8
Compare
Signed-off-by: MarzellT <[email protected]>
Signed-off-by: MarzellT <[email protected]>
Signed-off-by: MarzellT <[email protected]>
… information to the config Signed-off-by: MarzellT <[email protected]>
Signed-off-by: MarzellT <[email protected]>
Signed-off-by: MarzellT <[email protected]>
Signed-off-by: MarzellT <[email protected]>
Signed-off-by: MarzellT <[email protected]>
* adds first documentation draft * renames config-mapping to example-config-mapping * removes monitor_variables from config * small refactorings Signed-off-by: MarzellT <[email protected]>
…on ocpp config Signed-off-by: MarzellT <[email protected]>
…ll refactoring Signed-off-by: MarzellT <[email protected]>
Signed-off-by: MarzellT <[email protected]>
Signed-off-by: MarzellT <[email protected]>
MarzellT
force-pushed
the
tm-adding-ocpp-configuration-module
branch
from
83f3de8 to
10db827
Compare
Describe your changes
Issue ticket number and link
Checklist before requesting a review