New controller for flexibility trading with Levl Energy #2873
Conversation
remove physical soc bounds and target grid setpoint from config
LevlControlRequest
variables; fix efficiency calculation
…emove empty comments at line endings
handling in impl; move request handling to before_process_image event; clean up files
LastRequestTimestamp channel; add comments
can be analyzed in time db
well for realized battery value of last request
handleRequest; add logging and set log-level to debug; auto-formatting
calculation; fix swinging of remaining energy around zero;
| var sellToGridLimit = this.getSellToGridLimit().getOrError(); | ||
| var influenceSellToGrid = this.getInfluenceSellToGrid().getOrError(); | ||
|
|
||
| var essCapacityWs = essCapacity * 3600L; |
There was a problem hiding this comment.
There are a number of "magic numbers" (e.g., 3600L, 100.0 for percentage conversions). These constants should be defined as named constants to enhance readability and avoid repetition.
| public enum ChannelId implements io.openems.edge.common.channel.ChannelId { | ||
| REMAINING_LEVL_ENERGY(Doc.of(OpenemsType.LONG).persistencePriority(PersistencePriority.HIGH) | ||
| .text("energy to be realized [Ws]")), | ||
| LEVL_SOC(Doc.of(OpenemsType.LONG).unit(Unit.WATT_HOURS).persistencePriority(PersistencePriority.HIGH) |
There was a problem hiding this comment.
Some of the channel names could be more descriptive, particularly for states or conditions. For example, LEVL_SOC could be renamed to LevlEnergyStateOfCharge for clarity.
There was a problem hiding this comment.
I renamed some of them to clarify Soc = state of charge.
LevlEnergy could be confusing because it could be the energy for levl as well as the company name.
Please let me know if some channel names are still unclear.
| var nextPucSocWs = pucSocWs - Math.round(Efficiency.apply(pucBatteryPower, efficiency) * cycleTimeS); | ||
|
|
||
| // levl calculation | ||
| var levlBatteryPower = 0; |
There was a problem hiding this comment.
The naming of variables like pucBatteryPower, levlBatteryPower, etc., is clear, but using more specific names might help others quickly understand the context (e.g., primaryUseCaseBatteryPower and levlBatteryDischargePower).
There was a problem hiding this comment.
In our implementation, we followed the OpenEMS view for signs, in which OpenEMS is located in the center of the system. If something enters the system (e.g. battery is discharged), this is positive. If it leaves the system (e.g. battery is being charged), it is negative.
Writing out Puc would be an option, but we have decided to use the abbreviation for reasons of readability, variable length and the due to the number of uses and explain it accordingly at the beginning.
However, I am flexible about this :)
| @@ -0,0 +1,39 @@ | |||
| package io.openems.edge.levl.controller.common; | |||
|
|
|||
| public class Efficiency { | |||
There was a problem hiding this comment.
There is a duplication of logic between the apply() and unapply() methods in the Efficiency class. Both methods perform similar operations based on whether the value is positive or negative, applying the efficiency percentage. This duplicated logic can be refactored into a shared private method to reduce code repetition and improve maintainability.
There was a problem hiding this comment.
Good point. There was also a mistake in the method description I corrected.
| @@ -0,0 +1,140 @@ | |||
| package io.openems.edge.levl.controller; | |||
|
|
|||
| import com.google.gson.JsonObject; | |||
There was a problem hiding this comment.
Do not use gson.JsonObject. Instead use OpenEMS'es own Json Methods
There was a problem hiding this comment.
Now I use JsonUtils within the parseFields method. However, the OpenEMS'es own JsonrpcRequest.getParams() returns an gson JsonObject.
If there's a OpenEMS way to avoid this, I would appreciate a short hint :)
| var dischargeEnergyLowerBoundWs = pucSocWs - essCapacityWs; | ||
| var dischargeEnergyUpperBoundWs = pucSocWs; | ||
|
|
||
| var powerLowerBound = Efficiency.unapply(Math.round(dischargeEnergyLowerBoundWs / cycleTimeS), efficiency); |
There was a problem hiding this comment.
Maybe import Math statically? :)
| import io.openems.edge.common.component.OpenemsComponent; | ||
| import io.openems.edge.controller.api.Controller; | ||
|
|
||
| public interface ControllerEssBalancing extends Controller, OpenemsComponent { |
There was a problem hiding this comment.
Please use // to format the Channels properly so it is better readable (see other Channel Implementations)
This Comments is applicable for all Files
There was a problem hiding this comment.
Done. Also in the OpenEMS Framework testing class.
| * @param cycleTimeS the configured openems cycle time [seconds] | ||
| * @return the levl battery power [W] | ||
| */ | ||
| protected int calculateLevlBatteryPower(long remainingLevlEnergyWs, int pucBatteryPower, int minEssPower, |
There was a problem hiding this comment.
Many of the applyXYZToLevlPower methods in calculateLevlBatteryPower share similar logic and could be consolidated into a single method with parameters to increase readability and reduce code duplication.
There was a problem hiding this comment.
I moved the return operation (max(min(power, upperBound), lowerBound)) into a private method called applyBound().
In my opinion, the other lines do not lend themselves to being consolidated into a method, considering readability.
But if you want to talk about a specific point, please let me know.
Thank you for your review!
|
Could you please ellaborate what the following means:
Which test scenarios did you execute and which OpenEMS test framework do you refer to? If you refer to the tests in |
@parapluplu Thanks for the question. Of course by "OpenEMS test framework" I meant the tests in BalancingImplTest using OpenEMS' own test framework "ControllerTest". It's described as a "generic test framework for OpenEMS". Since it tests the whole controller and multiple cycles, I mentioned it separately to differentiate between the single tested methods within ControllerEssBalancingImplTest. So yes, my description might be a bit misunderstood :-) We also tested the controller within our own development environment using several simulated OpenEMS edge instances. |
| } | ||
|
|
||
| //Just for testing | ||
| public LevlControlRequest(int sellToGridLimitW, int buyFromGridLimitW, String levlRequestId, String timestamp, |
There was a problem hiding this comment.
package level visibility should be good enough then, isn't it?
There was a problem hiding this comment.
protected would even do. I adjusted this.
There was a problem hiding this comment.
protected is more than package level visibility, see https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html
But protected is good as well :)
| import java.time.LocalDateTime; | ||
| import java.util.Objects; | ||
|
|
||
| public class LevlControlRequest { |
There was a problem hiding this comment.
Would be good to put a version on this request, so it's easier to change the API version eventually
There was a problem hiding this comment.
You're right. It's definitely something we'll add at a later date. For the beta it will be fine.
There was a problem hiding this comment.
I'm okay with this approach, but it eases it if it's built in from the beginning
| import java.time.LocalDateTime; | ||
| import java.util.Objects; | ||
|
|
||
| public class LevlControlRequest { |
There was a problem hiding this comment.
What's the motivation to perform parts of the controlling via an external API and not within OpenEMS?
There was a problem hiding this comment.
Yes, it's about virtual power plants. I can imagine this being implemented within OpenEMS, with a virtual power plant coordinator running OpenEMS and multiple batteries running OpenEMS connected to the power plant. That’s the reason for my question.
This question isn’t intended to block or hinder the merge request; it’s simply out of curiosity.
There was a problem hiding this comment.
Hey guys, sorry for the late response. Levl enables battery owners to participate in the electricity market, making more efficient use of the batteries and reducing amortisation costs. At the same time, the electricity grid is relieved as electricity is bought when it is cheap (a lot of renewables energy in the grid) and sold when demand is high (coal and gas is needed).
Unfortunately, this is not quite so simple from a regulatory perspective, as battery storage systems behind-the-meter, especially with a primary use case such as optimising self-consumption, requires a distinction to be made between self-consumption and temporarily stored energy. This is the only way the energy supplier can correctly bill the quantities purchased. For this reason, we act as an aggregator with Levl.
In the end, this controller is comparable to various other controllers that make it possible to use an external service on the hardware and/or software side to optimise your own system.
To come back to your idea @parapluplu: If regulation were left out of the equation, that might be possible. But you would still need someone to run and manage this coordinator centrally. However, Levl's long-term goal is to enable all battery owners, regardless of the EMS used, to become part of a virtual power plant with their system.
At the moment we're working with a lot of battery owners who are using OpenEMS as well as working directly with Fenecon. For this reason we're looking forward to becoming part of the OpenEMS community :-)
There was a problem hiding this comment.
Hey,
Just had a quick glance.
Here are my 2 cents, listed down below.
Also: Please consider to rename the package to something like
"io.openems.edge.controller.ess.balancing.levl"
or something like that,
instead of io.openems.levl.controller
Helps to identify and categorize the controller
|
|
||
| protected static Clock clock = Clock.systemDefaultZone(); | ||
|
|
There was a problem hiding this comment.
Instead of useing Clock.systemDefaultZone(),
Consider useing the clock of the ComponentManager , since (all) components (FeneconBatteryExample), useing a clock, refer to the Clock of the ComponentManager (or a ClockProvider in general).
Consider applying this to the LevelControlRequest as well, if needed/possible.
Also: This allows compatability JUnit Tests and TimeLeapClock (example, FeneconBatteryTest).
| private long applyBound(long power, long lowerBound, long upperBound) { | ||
| return max(min(power, upperBound), lowerBound); | ||
| } |
There was a problem hiding this comment.
Consider useing the TypeUtils.fitWithin() methods
And introduce a new TypeUtils.fitWithin() method returning and useing long values.
| @Designate(ocd = Config.class, factory = true) | ||
| @Component(name = "Controller.Levl.Symmetric.Balancing", immediate = true, configurationPolicy = ConfigurationPolicy.REQUIRE) | ||
| @EventTopics({ EdgeEventConstants.TOPIC_CYCLE_BEFORE_PROCESS_IMAGE, EdgeEventConstants.TOPIC_CYCLE_AFTER_WRITE, }) |
There was a problem hiding this comment.
CodeFormatting, See FeneconBatteryExample
There was a problem hiding this comment.
maybe a stupid question, but if checkstyle does not complain code formatting should be ok, shouldn't it?
There was a problem hiding this comment.
@parapluplu You are right. If checkstyle does not complain, the code formatting should be ok.
Regardless of this, using the code formatting like in FeneconBatteryExample has some advantages. For example, for me it took 2-5s to understand the configuration of the fenecon battery. In your code it took me 15-20s. This is, because it took me some time to mentally divide the lines into several lines. This is how may brain is trained (because of other OpenEMS code). Other developers may have the same problem.
As we all want to work efficiently on the same code base it would be nice if we format the code more or less identical.
And in the end this issue should be reflected by an updated set of checkstyle rules..
Hey @DerStoecki, thanks for your 2 cents and advice. I have implemented your notes into the controller. Especially the TimeLeapClock was new to me, so thank you very much. I think we will use this in a later version for some further testing. On the package renaming: We used io.openems.edge.levl.controller as the package name because we were inspired by other controllers like io.openems.edge.kostal.piko. |
|
Hi @DennisLevl, can you please allow Commits for me. This would allow me to effectively work on the PR. Thanks! |
Unfortunately, this does not work for forks owned by an org (in this case, levlenergy. See https://github.com/orgs/community/discussions/5634). However, I have added you to the fork as a member. |
Adds new controller for flexibility trading with Levl Energy
General Information
This pull request introduces a new controller that enables OpenEMS and Fenecon customers to trade their flexibility using the Levl Energy platform. This functionality is currently in the beta stage and will be refined based on feedback and additional testing.
How it works
This controller receives charging/discharging instructions from Levl Energy and executes these taking into account local optimization and various constraints (such as state of charge, grid limits, etc.).
Testing
The business logic has been covered by unit tests. The controller has also been tested using multiple test scenarios within the OpenEMS test framework to ensure functionality and reliability.
Please review the changes and provide feedback. Once approved, this will be ready for merge. Thank you in advance!