Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Energy Management integration of OCPP and API modules #872

Merged
merged 34 commits into from
Nov 7, 2024

Conversation

Pietfried
Copy link
Contributor

@Pietfried Pietfried commented Sep 22, 2024

This PR refactors the energy management integration of the API module and the OCPP modules.

A little bit of energy management background

The evse_manager interface exposed a command set_external_limits to apply EVSE specific limits received from the OCPP CSMS or the API module. If both modules (API, OCPP) use this command to write limits, they override each other. This command was therefore removed from the evse_manager interface.

The Energymanagement implementation was already designed to use the external_energy_limits interface and EnergyNode modules instead of that to build up energy trees. This design is more flexible and can be customized for different charging station setups. The API and OCPP modules have been changed to require the external_energy_limits interface and use this to populate the charging limits they receive via their respective channels.

This introduces a breaking change to the Energymanagement integration in EVerest. It is now required to connect modules that implement the external_energy_limits interface to the API and OCPP modules in order to apply the received limits.

A recommended setup is to load one EnergyNode module per EVSE and an additional one representing the energy sink for the whole charging station (addressed by EVSE id zero in OCPP). These EnergyNode modules must be configured with an EVSE mapping in order to allow the API or OCPP modules to retrieve the correct evse id:
https://everest.github.io/nightly/general/05_existing_modules.html#tier-module-mappings

Summary of changes

  • Adjust manifests of API and OCPP modules to optionally require (0-128) modules implementing the external_energy_limits interface
  • Changed the integration of charging limits in these modules to use the new requirement
  • Changed all OCPP configuration files to by loading the additional EnergyNode modules and to address the changes required for the module connections
  • Added documentation to all modules to explain the energy management integration

Related PRs

EVerest/everest-framework#206
EVerest/everest-utils#154

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

modules/API/API.cpp Outdated Show resolved Hide resolved
config/config-sil-ocpp.yaml Outdated Show resolved Hide resolved
modules/API/API.cpp Outdated Show resolved Hide resolved
modules/API/API.cpp Outdated Show resolved Hide resolved
modules/API/API.cpp Outdated Show resolved Hide resolved
modules/OCPP/OCPP.cpp Outdated Show resolved Hide resolved
Comment on lines 167 to 176
- module_id: evse_manager_1_sink
implementation_id: external_limits
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of solution, i.e. to plugin one more instance of an EnergyNode mode in between the energy flow graph is pretty overkill! It is a complete running process, which won't do more than just receive a command and forward it as some limits. Maybe we should consider to have a single module for the energy flow which has a much more flexible and expressive configuration.

modules/OCPP/doc.rst Outdated Show resolved Hide resolved
@Pietfried Pietfried linked an issue Oct 7, 2024 that may be closed by this pull request
@Pietfried Pietfried force-pushed the feature/energymgmt-integration-ocpp-and-api branch from faeef1e to b88570e Compare October 10, 2024 13:00
modules/API/API.cpp Outdated Show resolved Hide resolved
@Pietfried Pietfried force-pushed the feature/energymgmt-integration-ocpp-and-api branch 2 times, most recently from 78f2c21 to a904613 Compare October 14, 2024 16:16
@Pietfried Pietfried force-pushed the feature/energymgmt-integration-ocpp-and-api branch from 9a2164b to 85c918b Compare October 15, 2024 09:43
@Pietfried Pietfried requested a review from a-w50 October 15, 2024 09:48
@Pietfried Pietfried marked this pull request as ready for review October 15, 2024 09:48
interface: external_energy_limits
min_connections: 0
max_connections: 1
max_connections: 129
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify that the evse_sink vector does not contain multiple elements with the same evse mapping

@Pietfried Pietfried added the include-in-release Tag that this PR should be included in the current merge window for the upcoming release if possible label Oct 16, 2024
Copy link
Contributor

@barsnick barsnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some typos found while browsing the doc markdown.

modules/OCPP/doc.rst Outdated Show resolved Hide resolved
modules/OCPP/doc.rst Outdated Show resolved Hide resolved
modules/OCPP/doc.rst Outdated Show resolved Hide resolved
modules/OCPP/doc.rst Outdated Show resolved Hide resolved
modules/OCPP201/doc.rst Outdated Show resolved Hide resolved
modules/OCPP201/doc.rst Outdated Show resolved Hide resolved
modules/OCPP201/doc.rst Outdated Show resolved Hide resolved
modules/OCPP201/doc.rst Outdated Show resolved Hide resolved
mhei added a commit to mhei/remotechargeport that referenced this pull request Oct 21, 2024
…erface

This adapts for the following PR in everest-core:
- EVerest/everest-core#872
- EVerest/everest-core#924

Signed-off-by: Michael Heimpold <mhei@heimpold.de>
Copy link
Contributor

@a-w50 a-w50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From perspective of code style, I can approve this PR.
If possible I might factor out the duplicated code, but it is not clear yet how to do that - i.e. how multiple modules can share some unit of functionality.

auto& evse_energy_sink = this->get_evse_sink_by_evse_id(evse_id);

this->mqtt.subscribe(cmd_set_limit, [&evse_manager_check = this->evse_manager_check,
&evse_energy_sink = evse_energy_sink](const std::string& data) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&evse_energy_sink = evse_energy_sink should be the same as &evse_energy_sink

modules/OCPP/OCPP.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
@Pietfried Pietfried removed the include-in-release Tag that this PR should be included in the current merge window for the upcoming release if possible label Oct 23, 2024
mhei added a commit to mhei/remotechargeport that referenced this pull request Oct 26, 2024
…terface

This adapts for the following PR in everest-core:
- EVerest/everest-core#872

Signed-off-by: Michael Heimpold <mhei@heimpold.de>
@Pietfried Pietfried self-assigned this Oct 29, 2024
@Pietfried Pietfried force-pushed the feature/energymgmt-integration-ocpp-and-api branch from b2d33cf to 126a345 Compare October 30, 2024 09:53
Pietfried and others added 7 commits November 4, 2024 15:42
* Remove set_external_limits from evse_manager inteface
* Change requirement to apply limits of API and OCPP modules from evse_manager to external_energy_limits

TODOs:
* Adjust all EVerest configuration files
* Add documentation to API and OCPP modules
* EnergyManager currently seg faults

Signed-off-by: pietfried <pietgoempel@gmail.com>
Signed-off-by: pietfried <pietgoempel@gmail.com>
Signed-off-by: Piet Gömpel <pietgoempel@gmail.com>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…of children to unique_ptr

Signed-off-by: Piet Gömpel <pietgoempel@gmail.com>
…r the energy management

* Using the mapping of the requirement to determine the connect requirement to call based on the evse id

Signed-off-by: Piet Gömpel <pietgoempel@gmail.com>
Signed-off-by: Piet Gömpel <pietgoempel@gmail.com>
cmake/ev-cli.cmake Outdated Show resolved Hide resolved
cmake/everest-generate.cmake Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP/manifest.yaml Show resolved Hide resolved
modules/OCPP201/manifest.yaml Outdated Show resolved Hide resolved
@hikinggrass
Copy link
Contributor

hikinggrass commented Nov 5, 2024

TODO: needs updates in dependencies.yaml for everest-framework and everest-utils (once versions are tagged there)

that would be:
everest-framework: v0.18.0
everest-utils: v0.4.0

Signed-off-by: Piet Gömpel <pietgoempel@gmail.com>
…g external_energy_limits reference by evse id

Signed-off-by: Piet Gömpel <pietgoempel@gmail.com>
@@ -1,4 +1,5 @@
add_subdirectory(can_dpm1000)
add_subdirectory(external_energy_limits)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hikinggrass probably we can include this only when either API, OCPP or OCPP201 are build but Im not quite sure how. Can you help with that?

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
modules/API/API.cpp Outdated Show resolved Hide resolved
modules/OCPP/OCPP.cpp Outdated Show resolved Hide resolved
…tegration-ocpp-and-api

# Conflicts:
#	lib/staging/CMakeLists.txt

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@SirVer
Copy link
Contributor

SirVer commented Nov 6, 2024

will review today!

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Copy link
Contributor

@SirVer SirVer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for Rust & Bazel.

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@Pietfried Pietfried merged commit 01dee7b into main Nov 7, 2024
11 checks passed
@Pietfried Pietfried deleted the feature/energymgmt-integration-ocpp-and-api branch November 7, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Integration of Charging Limits for EVSEs
6 participants