Add timeframe controller to reach specified SoC in specified time frame #2694
Conversation
|
Following this PR |
Femumme
force-pushed
the
feature/TimeframeController
branch
from
1c7e687
to
42e913a
Compare
|
I just rebased on the latest changes of the development branch - just out of curiosity, what do I have to do to get this merged? I already use this controller in my own openEMS instance and it is working like a charm. I'm planning on developing more features - and of course I could just keep track of them on my own fork of openEMS, but that way nobody else can use them - and I have to keep them up to date, which also takes some effort. |
|
@sfeilmeier Are you even interested in any collaboration? |
|
That's good to know! I understand that you guys got a lot on your plate. What can I do to make the review easier for you? I will update the branch to the latest changes on the development branch. |
@sfeilmeier can you assign me as a reviewer or isn't this neccessary? |
@Femumme Hard to say. We don't have a clear and good review process yet. Our coding guidelines are a first step: https://openems.github.io/openems.io/openems/latest/contribute/coding-guidelines.html Without looking at the code, how does this controller relate to https://github.com/OpenEMS/openems/tree/develop/io.openems.edge.controller.ess.fixstateofcharge? |
@sfeilmeier it has the "feature" that one can define a Timeframe in which the Storage shall reach the Targes SoC instead of just "stubborn" charging it. - In my honest Opinion i do not know in which Case this should be neccessary as the "Prepare Battery Extension" already serves this purpose - in the Prepare Battery Extension one can also configure to which SoC it shall Charge/Discharge which is not the Case in this PR. |
@Sn0w3y Thank you for taking a look! The target SoC can be set via the "targetSoC" config value, or via UI (which ultimately also changes the config value). What I was missing in the "Prepare Battery Extension" was the ability to set the start and end time of the charging/discharging phase. This is useful when you have a time-of-use tarif and if you want to discharge during high-cost times or charge during low-cost times. I saw that you can set the end-time, but the start-time is somewhat unclear to the end-user. Ultimately, I want to add an "Auto"-Mode, which would make use of a time-of-use-tarif and checks for those high-cost / low-cost periods to charge or discharge during those times automatically. Probably at that point this controller would make way more sense than it does right now. |
@sfeilmeier Thanks for pointing to the developer guideline, I will check the list and make sure I did everything accordingly. Regarding the prepare battery extension, @Sn0w3y also mentioned the similarity. I explained to him why I think that it differs in a previous reply. |
Femumme
force-pushed
the
feature/TimeframeController
branch
from
71bbb1d
to
2ed40e5
Compare
Also already Implemented in the ToU Controller for Tibber for example - if i am honest i still do not see the Point - do not get me wrong please. Every ToU (Time-of-Use) Tariff has its own API so basically you always need to fetch the Data from the APIs - how would this (your) Controller than differentiate from "ToU Tibber" Controller for example if you implement AUTO mode? Greetings |
There was a problem hiding this comment.
General Feedback already provided. See here:
I’ve completed the initial review of the pull request as you requested. Since this is my first review, I’d appreciate it if you could take a look and provide any feedback or advice on how I can improve in the future. I’m eager to learn and make sure my reviews are as helpful and thorough as possible. Also you should make a Decision if it makes sense to follow this approach as I already suggested in my Opinion it does not make sense to create a similiar Controller as the "Prepare Battery Extension" and "ToU Tibber" even if we need "more" intelligent ToU Controllers which charge/discharge the Batterys - but as we have it already in the Tibber my guess is, that it should be not as hard to copy-paste it to other ToU Tariffs, right?
Thanks for the opportunity to contribute!
| @@ -0,0 +1,6 @@ | |||
| FROM openjdk:21 | |||
There was a problem hiding this comment.
this file should not be in a PR.
| @@ -113,6 +114,7 @@ | |||
| bnd.identity;id='io.openems.edge.ess.core',\ | |||
| bnd.identity;id='io.openems.edge.ess.fenecon.commercial40',\ | |||
| bnd.identity;id='io.openems.edge.ess.generic',\ | |||
| bnd.identity;id='io.openems.edge.ess.mr.gridcon',\ | |||
There was a problem hiding this comment.
has been removed acutally:
MR Gridcon: move obsolete bundle to archive
See https://github.com/fenecon/openems-archive/tree/main/io.openems.edge.ess.mr.gridcon
There was a problem hiding this comment.
Please have a look at other .classpath files and change it accordingly. Also run "prepare-commit.sh" in tools Folder before creating a PR please as this fixes exactly this "Problems". Also always apply "Strg-Shift-F" and "Strg-Shift-O" if using Eclipse.
| private volatile Timedata timedata = null; | ||
|
|
||
|
|
||
| private final Logger log = LoggerFactory.getLogger(io.openems.edge.controller.ess.timeframe.ControllerEssTimeframeImpl.class); |
There was a problem hiding this comment.
This Line could be Changed to:
private final Logger log = LoggerFactory
.getLogger(ControllerEssTimeframeImpl.class);
Also it is not used in the Code as far as i can see
| int targetCapacity = maxEssCapacity * targetSoC / 100; | ||
| int currentCapacity = maxEssCapacity * currentSoC / 100; | ||
|
|
||
| float timeframeInHours = (end.getTime() - current.getTime()) / 1000f / 60f / 60f; |
There was a problem hiding this comment.
Maybe use a private static final for the Milliseconds to Hours:
private static final float MILLISECONDS_TO_HOURS = 1000f / 60f / 60f;
| */ | ||
| protected static Integer getAcPower( | ||
| ManagedSymmetricEss ess, | ||
| Integer fallback_ess_capacity, |
There was a problem hiding this comment.
fallbackEssCapacity - please watch or keep the Formatting of the Variables as in the other Code
| String startTime, | ||
| String endTime | ||
| ) throws InvalidValueException { | ||
| Date start = getDateFromIsoString(startTime); |
There was a problem hiding this comment.
Instead of using Date, the method could work directly with Instant, which is more modern and straightforward for handling time-based logic.
Instant currentInstant = Instant.now();
Instant startInstant = OffsetDateTime.parse(startTime, DateTimeFormatter.ISO_DATE_TIME).toInstant();
Instant endInstant = OffsetDateTime.parse(endTime, DateTimeFormatter.ISO_DATE_TIME).toInstant();
Then you also can get rid of:
getDatefromIsoString method
| Date start = getDateFromIsoString(startTime); | ||
| Date end = getDateFromIsoString(endTime); | ||
| Date current = new Date(); | ||
| if (current.after(start) && current.before(end)) { |
There was a problem hiding this comment.
The check for whether the current time is within the start and end times should be done early, allowing for an immediate return if the ESS is not active. This reduces the complexity of the code by avoiding unnecessary nesting.
| @AttributeDefinition(name = "Ess-ID", description = "ID of Ess device.") | ||
| String ess_id(); | ||
|
|
||
| @AttributeDefinition(name = "Fallback ESS Capacity", description = "Capacity of the ESS in Wh. Serves as Fallback, if ESS capacity can not be read from ESS.", required = false) |
There was a problem hiding this comment.
Why should the SoC should not be able to be read? It should be set in the Specific ESS i guess. Also if this is neccessary why is it not required ? If you could make required to "True" you could spare:
if (essCapacity <= 0) { throw new InvalidValueException( "Could not determine ESS capacity and no valid fallback capacity was configured."); }
as then the Fallback would always be there.
It would also be good to seperate the two cases e.g:
- ESS Capacity not readable and no Fallback configured
- ESS Capacity not readable using Fallback
and provide user feedback (log)
| package io.openems.edge.controller.ess.timeframe; | ||
|
|
||
| public enum Mode { | ||
| MANUAL, OFF; |
There was a problem hiding this comment.
I guess "ON", "AUTO", "OFF" would be better as "ON" charges as it should do as configured. etc.
I wasn't aware of that feature in the Tibber ToU, I will need to take a deeper look. Thanks again for the hint - and thank you for the other feedback provided, I will go through your comments and adjust the implementation :-) |
|
Sorry for hijacking but I'm very interested in this as well. Until now, I've modified the fixActivePower controller to charge and discharge at certain times, but I've done sloppy work and does require some manual input. So I'm eager to see this functionality, with ToU (Entsoe in my case) happening soon! |
|
Maybe mark this as WIP @Femumme |
@Femumme: I still did not find time to look into the code, but if - as Sn0w3y says - there are similarities with existing ones, should we consider adding the missing functionality somewhere else? Regarding the feature set of the current time-of-use implementation: see the FENECON documentation which documents it in more detail: https://docs.fenecon.de/de/fems/fems-app/OEM_App_TOU.html I also held a webinar on the topic recently which might be interesting: https://www.pv-magazine.de/webinare/dynamische-stromtarife-in-heim-gewerbe-und-industrie-ueber-ki-basierte-prognosen-intelligent-nutzen/ I am working on improving (= rewriting) the time-of-use Controller though, to enable additional features like discharging from ESS to grid or charging an electric vehicle in combination with an ESS. Code is not yet ready to be published, but working already on my private system... |
# Conflicts: # ui/src/app/edge/live/live.module.ts # ui/src/app/shared/components/modal/modal-line/modal-line.ts # ui/src/app/shared/service/defaulttypes.ts # ui/src/app/shared/type/widget.ts
This PR adds a new controller that allows the user to easily set a specified timeframe to reach a target SoC.
The main use-case for this is to sell during expensive times and charge during low-cost times.
I'm planning on adding an automatic mode as well, that uses a TimeOfUse Tarif to determine the most cost effective hours to charge / discharge.
Any feedback is welcome! Since this is my first contribution, I'm also happy to receive any advice what I could've done better :-)