Adding yeti simulator #862
Conversation
MarzellT
force-pushed
the
tm-adding-yeti-simulator
branch
from
b1b9863
to
b69e1d0
Compare
MarzellT
requested review from
SebaLukas,
Pietfried,
hikinggrass and
corneliusclaussen
as code owners
MarzellT
force-pushed
the
tm-adding-yeti-simulator
branch
4 times, most recently
from
8529f5f
to
7ac3d30
Compare
corneliusclaussen
force-pushed
the
tm-adding-yeti-simulator
branch
2 times, most recently
from
59b66bc
to
9583e7c
Compare
Signed-off-by: MarzellT <tobias.marzell@pionix.de>
corneliusclaussen
force-pushed
the
tm-adding-yeti-simulator
branch
from
62767a7
to
d2818bf
Compare
There was a problem hiding this comment.
I am divided on the PR. I think that the code basically works, but I find the architecture confusing.
I think that the complete code from the simulation.hpp can also be moved and executed in the YetiSimulator.cpp.
This saves the many get functions and the module structure would also be like other EVerest modules. And I think that the clarity will not suffer.
I also don't like the large ModuleState struct. This could be reduced by moving the code to YetiSimulator.cpp and define the individual variables and structs, such as PowermeterData and SimulationDatadirectly in the YetiSimulator class.
There was a problem hiding this comment.
I see no documentation, so delete this
| const auto capabilities = types::evse_board_support::HardwareCapabilities{ | ||
| .max_current_A_import = 32.0, | ||
| .min_current_A_import = 6.0, | ||
| .max_phase_count_import = 3, | ||
| .min_phase_count_import = 1, | ||
| .max_current_A_export = 16.0, | ||
| .min_current_A_export = 0.0, | ||
| .max_phase_count_export = 3, | ||
| .min_phase_count_export = 1, | ||
| .supports_changing_phases_during_charging = true, | ||
| .connector_type = types::evse_board_support::Connector_type::IEC62196Type2Cable}; |
There was a problem hiding this comment.
Move this to a set_default_capabilities() function
|
|
||
| void ev_board_supportImpl::handle_enable(bool& value) { | ||
| auto& module_state = mod->get_module_state(); | ||
| if (module_state.simulation_enabled && !value) { |
| {"irmsN", powermeter_data.irmsN}}; | ||
| } | ||
|
|
||
| } // namespace module::state |
|
|
||
| namespace module { | ||
|
|
||
| template <typename YetiSimulatorT, typename BoardSupportT> class Simulator { |
There was a problem hiding this comment.
Why did you define a template class?
There was a problem hiding this comment.
I think this code should be in YetiSimulator.cpp and not in a separated header file.
| .max_phase_count_export = 3, | ||
| .min_phase_count_export = 1, | ||
| .supports_changing_phases_during_charging = true, | ||
| .connector_type = types::evse_board_support::Connector_type::IEC62196Type2Cable}; |
There was a problem hiding this comment.
Designated initializers are only supported in C++20 and above.
(Also below with return {.ampacity = Ampacity::None};.)
There was a problem hiding this comment.
See my comments in #887 (comment). Not sure why this is another PR.
I created a working example in this branch: https://github.com/EVerest/everest-core/tree/refactor/cleanup_yeti_sim_error
Describe your changes
Issue ticket number and link
Checklist before requesting a review